Stephen Yu
@StephenYu2018All comments
- P@mayor-creator@StephenYu2018
Hi Paul,
Flexbox generally focuses much more on the positioning of its items rather than the sizing of its items. Giving size to flex items generally works the same way as giving size to most other elements.
I think the cards' children elements overflow downwards off the card itself when the browser's viewport is between 481px and 800px is because each card is set to a fixed width rather than
auto
(initial value when you don't set thewidth
orheight
). The card is to remain at that fixed height, but because the width of the card can't fit all their children, the children elements are squeezed and pushed downwards, even running off the card in this case. Also, by setting themax-width
of the card instead of thewidth
, thewidth
is initialized toauto
. The browser tries to fit all the card's within the viewport's width, even if it means decreasing the width of the cards and squeezing its children downwards.Also, when you use media queries to change from
flex: row;
toflex: column;
, you gavemax-width
inem
. I don't see a good reason for this and I think it would've been much less confusing to read, both for you and others, if you usedpx
, since most browsers display the viewport size inpx
.Marked as helpful - @mossabboujnah@StephenYu2018
I like the attention to detail on the colors and typography. Here are some improvements you can consider:
The QR code
<img>
should havealt
text that describes what the image shows. Maybe something like "QR code" or "QR code to Frontend Mentor website".Also, notice the border radius of
<img>
shown in the design. It's actually smaller than the border radius for the card itself as opposed to being the same border radius size. - @michaelhaaf@StephenYu2018
Check your CSS color values again. The style guide states that grayish blue is
hsl(220, 15%, 55%)
, but you have:body { ... color: hsl(218, 44%, 55%); }
So it would have seemed that you used the saturation for dark blue in the place of the saturation for grayish blue. You've also used the lightness for grayish blue in the place of the lightness for dark blue.
To improve your odds of avoiding these types of mix-ups, it helps to use CSS variables to assign names to values. This keeps those styles in one place and gives that color a connection to the style guide. For example, you could have:
:root { --white: hsl(0, 0%, 100%); --light-gray: hsl(212, 45%, 89%); --grayish-blue: hsl(220, 15%, 55%); --dark-blue: hsl(218, 44%, 22%); }
And then you could use the variables throughout your CSS:
body { ... color: var(--dark-blue); } main { background-color: var(--light-gray); ... } ... .modal { background-color: var(--white); } ... .subtitle { ... color: var(--dark-blue); // Not hsl(255, 100%, 0%) ... } ... @media only screen and (min-width: 500px) { main { background-color: var(--light-gray); ... } ... }
This reduces code duplication, makes your code more readable, and keeps the values that need to change for a single reason together. If any of your colors don't look correct for any reason, you could always look at the variable name used, then go to
:root
to see what color value it was assigned. All your color values are kept in one place (being:root
) and you can fix all instances of the same bug wherever the color is used.For your text not being in the correct font family, double-check to make sure that you copied the all the proper HTML tags from Google Fonts to your
index.html
. There are usually 3 different <link> tags for you to copy, not just 1.Also, there's no styles provided for the
.description
class. Check to make sure you have them in your CSS files on your device. And if you already do, then it's likely that you didn't commit them to git properly, or it didn't get pushed properly to GitHub.Marked as helpful - @momin-riyadh@StephenYu2018
I liked the sizing of the component, image and texts. I'd also like to give a few suggestions for what you can do in the future:
alt
attribute of<img>
represents alternative text that is displayed in place of the image when the browser cannot display that image for some reason/error. A better and more descriptive alt text for the<img>
would bealt="QR code to visit Frontend Mentor"
- Prefer using
rem
measurements instead ofpx
measurements for sizing.rem
calculates its measurement off the font size of the root element (which ishtml
). The font size of the root element is dictated by the browser and can be changed by the user. While the default is16px
on most browsers, users can choose to make it larger, possibly because most text would be too small to read. By sticking withpx
measurements, you remove that capability from the user, as the text won't scale larger when the user chooses larger font sizes. Instead the text will remain the same size.- Also, do not style
html
font size usingpx
to change how the root font size appears. That also removes that feature from the user as well. If you want the root font size of your page to appear at a different size:- Use percentage units
%
- Divide your apparent root font size with the actual default root font size
- Example: If I wanted my root font size to appear at 13px instead of 16px, I would do the following calculation:
13px / 16px = 0.8125 = 81.25%
- The final CSS property would be
html { font-size: 81.25%; }
- Use percentage units
- Also, do not style
Marked as helpful