Skip to content
OverviewSolutions
4
Comments
5

Jonathan

@JonMStevens105 points

I’m a mysterious individual who has yet to fill out my bio. One thing’s for certain: I love writing front-end code!

Latest solutions

Latest comments

  • Jonathan•105
    @JonMStevens

    Hi, Nice work on the project!

    Some suggestions regarding semantic HTML: The container div that contains the logo could be a header. The div that contains the calculator itself could be contained in a main element. The attribution could be a footer element. Your h2 elements that label the inputs could be actual label elements with "for" properties pointing to the inputs. The page should also have one h1 header. I made the logo my h1 when I did this project myself, but I haven't confirmed if that was the best practice.

    Outside of semantic HTML: In my opinion, if you select another tip button or if you press reset, the custom tip input should hide and the custom tip button should show again. Lastly, I think you could add another breakpoint in between the desktop and mobile view, or use relative units on the box class. If you shrink the screen size then some of the calculator starts to get cut off before the 375px media query takes effect.

    Those are all the suggestions I have. It works well and looks good. Nice job!

    Marked as helpful
  • Jonathan•105
    @JonMStevens

    Hey,

    Really nice project.

    I just have a few details to nitpick. Your header contains a nav which just contains an image. I think the purpose of the nav tag is mainly to include links to navigate around a site. Also, there's some text on the page that says "Built for modern use. You could put some letter-spacing on that to make it look closer to the design. The line-height looks a little different on the h1 in the design, too. These are really minor details though.

    Thanks for your comment on my project. Sorry that I don't have a good resource yet on responsive sites, but yours looks good to me. Nice work!

  • Jonathan•105
    @JonMStevens

    Hey,

    Nice work on this. The layout looks really good, and I think it's cool that you used grid areas.

    I agree with what the other poster said. I think it could look better if the whole thing was horizontally centered. One fix could be on the section selector just changing "margin: 70px 24px;" to "margin: 70px auto;" or something like "70px max(10px, 10%)" to make sure there's always a small margin when the screen shrinks (you could maybe play with the exact numbers though). Also, in my opinion one reason why the page starts to look cramped at smaller screen size, before the mobile breakpoint kicks in is that the headers start to wrap. Putting "white-space: nowrap;" on the header selector can fix this. This is just my opinion though. Lastly, I think the background color on the page is different to the one in the design.

    These are small details. Good job overall, and thanks for your comment on my project.

  • Jonathan•105
    @JonMStevens

    Hey this looks good so I'm just going to point out some small details about the button links that you could look at if you want.

    1. the hover states look to be sharing a few style properties. You could group these in one selector that they share so that you can easily change them all at once. The selector could be something like ".container-columns a:hover {" and contain all of those shared properties.

    2. the hover states seem to overwrite some of the properties on the anchor selector, even though they aren't changing the value. For example the anchor selector and the hover selectors both have: height: 47px; width: 150px; You don't need rewrite those properties.

    3. On hover the borders on the buttons seem to pop into place suddenly at the end of the transition. In my opinion this could look smoother if: they weren't included as part of the transition, and instead the transition was something like "transition: 500ms background-color ease-in" so only the background-color was part of the transition. OR you could outline the buttons with something that isn't part of the box model and won't take up space on the page like a box-shadow.

    Hope this is helpful, and thanks for your response on my project.

    Marked as helpful
  • Jonathan•105
    @JonMStevens

    I think you're missing the 'and' keyword on your media query. Your styles in the media query work for me when I edit line 67 in main.css to be:

    @media screen and (max-width: 1199px) {

    Marked as helpful