@Islandstone89
Posted
Hi. It looks good on both mobile and desktop, well done. Here are a few things to improve.
HTML:
-
Every webpage should have a
<main>that wraps the content(except header and footer). In this example, .qr-code could be the main element. -
Headings are hierarchical, so you should never start with a h3. Always start with a h1. One thing to note; there should only be one h1 on a page, which is the main heading. If you had a card with several headings, you should not use h1. But in this case, I think h1 is OK given there's only one heading on this page.
CSS:
-
It's common practice to write the CSS in a separate style.css file, then link to that in the
<head>. -
It's a good idea to have some sort of "CSS Reset" at the beginning. Here is one of the most popular ones.
-
Remove the
display: noneon the .qr-box. The card should not be hidden. It's not in this case, since it is overridden by you declaringdisplay: block. You should remove that one as well. The card is a div (which you should change to main, as mentioned), and so it is a block element by default. -
Changing the html font-size to 62.5% is not considered a good idea.
-
Remove fixed widths. You could have a max-width, set in REM, on the card.
@SkAliya
Posted
@Islandstone89 Hey Haberg , thanks for your suggestions i will modify, can you please help out, I didn't understand about html-font sizing can you explain in few words how to set rem units ,if you don't mind.
@Islandstone89
Posted
@SkAliya 1 rem equals to 16px by default. So, let's say you wanted something to be 24px, that would be 24 / 16 = 1.5 rem. Makes sense?
