@YariMorcus
Posted
Congratulations on completing this challenge Myndi!
I took a look at your solution, and I need to say that it looks pretty good! I can see that you took the time to create your component as close as looking to the designs that where given to you. This is not something I see from all people, so you should be proud on yourself!
Of course I do have some feedback for you, which I have been dividing into sections for more structure.
Section: HTML - feedback
-
Try to be as specific as possible in your
<html lang="en">attribute. So in this case, I would use:<html lang="en-us">. This is important for not only search engines, but also assistive technologies such as screen readers and browsers. Browsers use this tag to make sure it is using the correct dictionary in order to apply the right grammar and spelling (yes, there is a difference, you can consult the article of The Cold Wire for more information). Screen readers, for example, use this tag to apply the correct pronunciation of words. -
Try to use the semantic HTML 5 elements instead of the common block-level element called
div. A div (just like a span) does not convey any information to search engines and assistive technologies about what the content really is (unless you use ARIA, but that is something you should not be concerned with at the moment). What I did was the following: I added<main>as a direct child of<body>to indicate that this is the main content of the page. Within<main>, I added<article>to indicate that the QR code component standalone content is (it is content that you can share with others, without needing to provide them with other content on the page, in order for them to understand it). Within article I placed<header>to indicate that<img>is the header image of the component. As a sibling of<header>, I used<section>.<section>indicates that a group of content shares the same functionality (in this case, the heading and paragraph). I have to point out here that it might vary from other people. The main idea here is that you try to ask yourself what the purpose and functionality is of specific content. Based on this, you choose the corresponding semantic HTML 5 element. -
Try to avoid the use of the style attribute on elements. It is a bad practice because you are polluting your HTML file with CSS. You always should separate these technologies (HTML, CSS, and JavaScript) as much as you can). If you are going to use the style attribute, this makes it harder for other developers to read and edit your code, but also creates semantic problems, because you need to edit your HTML constantly to change the style of an element. Also, you cannot reuse styles such as a class in CSS (unless you do copy paste, which you should not be doing).
Section: CSS - Feedback
-
I see you are using
background-image: url('');andjustify-content: center;withinbodyelement selector. Is there a reason why you are using this? Or did you forget to remove it? If it is not serving a purpose, then I recommend you to remove it. Unnecessary styles always make your stylesheet unnecessarily larger, and developers have more code to read (even code that does not do anything). You even do yourself a favor with this haha. -
I see you are using a comment called
/* display: inline; */within.card-header. I think you forgot to remove it.
Section: CSS - What you did good
-
You are consistent with the color model you use (in this case, the HSL color model). Some people tend to use color names, hex, HSL, and RGB all at the same time.
-
You are using relative units (
vh,emand%) within CSS, which is always good from a responsive design perspective. -
You are using clear and descriptive class names.
Something else I do want to share with you, which I think is very important, is the one below.
I noticed in style.css a media query with the media feature max-width. I am not sure whether you made your design for mobile first, but always try to follow the 'mobile first' principle. This means that you should always design (develop) for mobile devices first, then tablets, and then laptops and desktops. If you are going the other way around, you might end up with (a lot of) frustration, because it becomes harder to make your website display correctly on smaller screens (trust me, I have been doing this in the beginning, and you really do not want to be there).
I hope you can do something with the feedback I gave you. If you have any more questions, feel free to ask me.
If I made a mistake somewhere in this post, feel free to correct me and keep building awesome things :D.
Marked as helpful
@Myntsu
Posted
@YariMorcus Thank you so much for kindly giving me general feedback. I will go through each of the points briefly.
HTML
-
This is my bad. I used the HTML code given by default. I should have used mine, as I didn't expect it to have any faults.
-
This one is funny. I thought of doing that initially, but I was like "meh, don't want to be unnecessary nerdy", and now here we are.
-
I have seen myself doing this quite few times. I tend to do it when there's only 1 specific thing to target. The only reason is because I don't want my CSS to be too long. Perhaps I should make another CSS for certain styles too?
CSS
Pretty much. I forgot to remove both of 1 and 2. Probably tired, or in a rush to finish. Perhaps both.
Mobile-first
I always have this in mind, but I always end building Desktop first, as Bootstrap (what I usually use) has simple and easy tools to fix Mobile incompatibilities.
My question is, is there any reason why Mobile first? At the end of the day you adjust for one another, right? I simply imagine myself designing in Mobile layout, to later find it looks awful in Desktop then having to re-adjust, but I might be wrong, since I have never tried it.
