Thank you for checking out this portfolio project: https://mixitupketterer.netlify.app/
The code can be retrieved from: https://github.com/giterdun345/cocktailAPI-mixitup/tree/master/mixitup
Any feedback is greatly appreciated and hope it might come in handy one day for yourself. Find some new cocktails or smoothie to make!
Hi @giterdun345 !
I think it looks good.
The links on the home page don’t work for me.
Hey @giterdun345, just a few suggetions:
- Do not make font sizes based on view port units only. Currently you have the font size for most content set to
2vw. When I narrow my browser all the way in the font gets way too small to be readable. And since you are using view port width I can’t manually resize the font (the only way to make the font bigger is to widen the browser). For the main content, I personally never go below
1em for font size. If you wanted it to scale a little depending on the browser width then you could do something like:
font-size: calc(1em + 1vw);
But notice that the minimum is still
1em. I know that you are trying to force the layout to remain the same as the browser narrows, but in order to do this you are having to make the text extremely small, which is an accessibility issue. Instead, you should use a responsive design so that the text/images collapse into one column at narrower widths. This way you will be able to keep the font size readable.
Using Firefox, I am seeing a horizontal scroll bar at all browser widths. Granted, it does not scroll much, but I think it would be better if it were not there at all.
There are certain browser widths in which the text “Search by the name of the cocktail:” is being cut-off by the image above it.
The search input needs a label.
It seems like you are using
<buttons> for links. For example, the “Learn More” button takes you to a new page, so it should be a link. The saying goes: “Buttons do things, links go places”. I’m assuming that the other two buttons below also need to be links since they seem to take you to a new page as well.
<section> tag should probably be a
<main> and then you should bring the
#content div inside the
<main>. You could then split the main content into
<sections> if you want but I don’t think it is necessary here, especially since you don’t have any headings to represent different sections in the main content.
You are relying on the browser’s default keyboard focus indicators, which while technically is acceptable (for now), does not really give a good focus indicator for everything on the page. For example, in my FF, I can’t really see the outline around any of the buttons. I would suggest that you customize the focus indicator (you can use the CSS
outline property, or even a
Thank you for the heads up, I had to add a file for production. Thanks again!
Interesting points, I haven’t considered many of it so thank you so much for your detailed feedback! I have a bit of playing around to do with those font calculations. Thanks again!
This topic was automatically closed 182 days after the last reply. New replies are no longer allowed.