Skip to content
OverviewSolutions
18
Comments
25
P

Jeremy Helsel

@JIH7Chicago670 points

Hi! I'm a Chicago based web developer and hobbyist video game developer. I love writing both front end and back end code, and I especially love the rush of seeing a big project come together. When I'm not programming, I love cooking, chess, reading and playing video games.

I’m currently learning...

Svelte, TypeScript, NextJS, C#, Unity, SASS, Python

Latest solutions

View more solutions

Latest comments

  • P
    Jeremy Helsel•670
    @JIH7Posted over 1 year ago

    Looks great! Like you said yourself, it's a pretty simple project, not a ton to improve on. The only thing I noticed in your code is there's an unused media query in your CSS.

    1
  • P
    Jeremy Helsel•670
    @JIH7Posted over 1 year ago

    Looking sleek and mobile friendly! Good semantic HTML markup and style rules. Really any feedback I could give are super small nitpicks. Starting with mobile styles and using media queries for tablet/desktop is generally considered a better workflow than starting with desktop and working your way down. It's something I've only somewhat recently started doing myself. The only other thing that I think could really help this project shine is a custom readme! Right now you've got the default Frontend Mentor README.md showing on your GitHub repo, and adding your own does wonders for making the project look better. I always find the README-template.md that comes with these projects to be a great jumping off point.

    Marked as helpful
    0
  • P
    Jeremy Helsel•670
    @JIH7Posted over 1 year ago

    Looking good overall! One issue I noticed, I believe you're trying to select the body element with the selector .body. The dot at the begginning makes this a class selector. So it would work for <body class="body"></body>. Changing it to just body would apply the styles in the way I think you're intending to. I downloaded your code and tried that and it put the container nice and centered on the screen.

    I've also noticed you have a white box shadow on the container, causing it to blend in with the white background. When a color is added, the borders of .container extend past the content. To get the box shadow working properly, try giving .container a proper width instead of a full screen width of 1440px. Something like 736px is more akin to the design. You will of course have to change the widths of the objects in between to avoid them squishing down since you're using percentages, but the page will look better because you did it. You probably also want to change the justify-content of .container to space-between. The flex rules you've set up on body will keep the container centered on the screen for you.

    You've got a solid start here and with some tweaks it could look really great! After that, try challenging yourself to learn CSS media queries to make it look good on mobile! That way, you can selectively apply CSS styles based on the screen size. Here's an article about that. Remember to hit f12 and open your browsers dev tools, then select different screens under "dimensions" at the top to see how it would look on different phones and tablets. Best of luck! Feel free to respond with any clarification questions

    0
  • P
    Jeremy Helsel•670
    @JIH7Posted over 1 year ago

    Hey! Awesome that you're starting your journey as a front end developer. Right now, there are a lot of ways you can improve this project. Your style tag isn't within the head tag, which could cause some issues. It also appears you're using qrcode.html file mostly for style, then loading the index.html file into an iframe. This is a pretty unusual approach and wouldn't really be considered best practice by most. If you would like to seperate your styles from the content, consider making a .css file and importing that into your html file with a <link> tag within the <head>. You can put your styles in there.

    The content in index.html also appears to be untagged. Just like with your body, iframe and image tags, you generally want all of your text to be within tags too. For instance "Improve your front-end skills by building projects" would be great inside of a heading 1 or <h1> tag. So something like <h1>Improve your front-end skills by building projects</h1>. The second line of text is great for a paragraph or <p> tag. This makes it easier for search engines like Google to understand your website, helps people with accessibility needs navigate your website, makes things easier to style, and will make it easier for you to work on a project you haven't touched in a while.

    I noticed you have a hover effect on the image, that isn't really required for this particular project if you would like to make it look like the design image.

    Also you can round the edge of the image with the css border-radius property. Something like border-radius: 10px; on the image would make it look more like the one in the design.

    I made a version of this as well if you'd like to see the code. Ignore the .scss files, that's a tool I used to help me write css and not something to worry about yet. The important files here are index.html and style.css. Live version of the site here.

    Let me know if you have any questions I can help with. Best of luck to you! Keep learning and you'll be making beautiful web pages before you know it :D

    3
  • P
    Jeremy Helsel•670
    @JIH7Posted over 1 year ago

    Looks great! One small oversight I noticed is that the price text is supposed to be green instead of gray. Otherwise very solid job replicating the design! Looks good on mobile and hover state on the button works well too. Great job!

    Marked as helpful
    0
  • P
    Jeremy Helsel•670
    @JIH7Posted over 1 year ago

    Overall looks good! I noticed you're not using the same font as the design. Check the included style-guide.md file for fonts used in the project. Then you can get whatever font you need from Google fonts and import it in the head of your HTML. Google fonts will actually prepare the necessary link statement and CSS rules on the right side of the screen.

    Also the heading on the white tiles looks like it's supposed to be some shade of gray instead of black and the heading text should be a little smaller. Colors used should also be in the style-guide.md. Great work!

    Marked as helpful
    0
View more comments