Skip to content
OverviewSolutions
7
Comments
13
Richard Ogujawa 👨‍💻
@RichardOgujawa

All comments

  • Richard Ogujawa 👨‍💻•170
    @RichardOgujawaPosted almost 2 years ago

    Hey Arsenije,

    First of all, I just want to say that I’m happy to see the improvements you made. Whether or not they were perfect doesn’t matter, all that matters is that you’re aware of and trying out new concepts.

    I’d also like to say thank you, because in the course of writing this comment I learned a few things myself, some of which I’ll share with you below.

    I didn’t make all the changes I wanted to make because I’m a little bit busy at the moment unfortunately, but the only remaining change really is just changing the font, and spacing (padding and margin) values from fixed values (pixels) to relative units (rem and em).

    Also, all the changes I made are in between comment tags that say ”Changes” at the top of it and “End of Changes” at the bottom.

    Anything commented out is code that you wrote that I changed, and anything typed in between the changes comment is code I added.

    You can find all the changes on your Github as pull requests.

    Some points to mention: The Picture element:

    • The picture tag didn’t work too well when I tried resizing the page on the live site, and I think that’s because you used both max-width and min-width when defining the media queries in the sizes attribute. There’s nothing wrong with using them both, it’s actually probably better to do it that way. For your media queries something I learned recently is that you can simply use min-width for mobile first designs or max-width for mobile first designs, and then for the screen sizes in between you could do media queries that specify a range, for example between 414px and 768px, which is where you’d utilise both min-width and max-width.
    • I proposed a fix in the HTML of simply taking out one of the media queries. I offered two methods to achieve the same result. The first one, which I commented out so it doesn’t disrupt the flow of the website, focuses on max-width and simply specifies that it wants an image of 384px to display on devices up to and smaller than 687px, and then the 300px after the comma simply means that for all other screen sizes display an image that is 300px. I gave the smaller screen size the bigger image width in this media query because the mobile image in the directory is wider than the desktop one, so the browser will choose it over the narrower desktop one when it’s choosing an image for the mobile view.
    • There was also an issue with the picture not being wide or tall enough to fit the component, which was simply fixed by giving the image element itself a width and height of 100% and an object-fit of cover so that it will crop however way it needs to to always fit the specified dimensions of 100% width by 100% height of the container element.

    Border-radius problem

    • To fix the border-radius problem, I simply took off the flex box properties you used on the product card in the mobile view, such as justify-content and flex-wrap. The border radius will only work If the container’s corners are wrapped around the corners of the content, but the flex box properties were pushing the content to the centre of the box, and the corners of the content were far from the corners of the product card. If you want to see what I mean, go into your original code, go into the mobile view for your site, and give your product card “outline:3px solid red” to see were it’s corners are in relation to the corners of your content.
    • Removing the flexbox properties, took away the gap between the containers corners and the corners of your content, and allowed the border-radius to therefore, affect it.
    • All that was missing now to see this effect is to add an “overflow:hidden”, so that the container will clip any content that extends outside of it.

    Minor changes

    • I’d recommend using a flex box instead of a grid for the layout. If you’re making something more complex, i.e. something that has more than one row or more than one column, then a grid is advised, but otherwise flexbox is a better option, is easier to code and generally requires less lines of code to get up and running.

    As always, if you want any help going over anything I said, just let me know:) Happy coding!

    Marked as helpful
    0
  • Richard Ogujawa 👨‍💻•170
    @RichardOgujawaPosted almost 2 years ago

    @Teles23 No worries man, more than happy to help! Best of luck with coding going forward, I'm sure with an attitude like the one you have right now you'll do great things:)

    0
  • Richard Ogujawa 👨‍💻•170
    @RichardOgujawaPosted almost 2 years ago

    Hi Guillherme!

    Hope you're keeping well:) First and foremost I really like how you've indented your HTML really makes it a lot easier to read, and I also like how you put headings in your HTML using comments to help make it easier to navigate. I also like how you used variables in your CSS, didn't see a lot of people do that in their coding solutions.

    However, I do have some suggestions for you. But these are just my opinion so feel free to take this on board or take it with a grain of salt, no hard feelings.

    In the HTML:

    • I'd recommend using an article tag for the nft preview component as opposed to a section tag. The article tag is a tag used for components that can be easily understood without any information around it. In otherwords you can look at it without it being in the context of the full site and it ill still makes sense. This allows it to be easily reusable and distributable. For example, blog posts, articles, components, widgets, etc. More on it here: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/article
    • I'd recommend looking into BEM Naming Conventions for naming classes. It makes it easier for you to write your class names without having to think too much about it, and it also makes it easier for other developers to understand your classes, because the naming convention is quite a logical one. There are two articles I'd recommend reading on it, and once you read them you're pretty much set. The first one talks about how to write BEM (https://codeburst.io/understanding-css-bem-naming-convention-a8cca116d252 ) and the second one talks about common problems you may run into when writing BEM and how to overcome them (https://medium.com/fed-or-dead/battling-bem-5-common-problems-and-how-to-avoid-them-5bbd23dee319)
    • I'd recommend against using a div to display an image if the image is not purely decorative, i.e. doesn't provide any meaningful information to the user, because you can't add alt text to it. This will mean that visually impaired people will have no way of knowing that you even have images on your website.
    • Also when it comes to images I'd recommend using a picture tag instead of just the img tag because it's a much better way to add responsive images to your websites instead of just relying on CSS. CSS simply scales up and scales down an image, so if you use only one image to cater for the fact that it would be on both larger screens and smaller ones, you'd have to use a high-resolution image in your src for the image tag. However, not all users need that high-resolution image, and would prefer to load a lower-resolution one because their device might be slow as is, or they may just be using a device with a smaller screen and therefore loading a high-resolution file would be overkill, because even though it's scaled down it's still a high-resolution file meaning it's a large file to load. More on all that here: (https://blog.bitsrc.io/why-you-should-use-picture-tag-instead-of-img-tag-b9841e86bf8b).
    • Icons and images (unless it's purely decorative) should have alt text attached to it. The only other time you may not want alt text on your icons is when it's beside text that already essentially describes what it is, such as an icon of a happy dog beside text that says 'happy dog' because if you include the alt text the screen reader would end up reading out "happy dog, happy dog.”
    • Also when writing your alt text write it as though you're describing the image on the screen to a blind friend so they feel like they too can see it.
    • HTML has tags for almost everything, there's an address tag for example, and there's also a time tag. All geared towards helping browsers better understand your website, it also is helps with making your site more accessibile. With that said I'd recommend wrapping the '3 days left' in a <time></time> tag.

    In the CSS:

    • If you don’t give your HTML CSS, there is a default stylesheet applied to your HTML, and it’s not always ideal, so if you want a better default stylesheet to work with I’d recommend using a CSS reset. It provides you a much better foundation for you to build your CSS on top of. There are a few of them online, but the one I use at the moment is Josh W Comeau’s and the only addition I would add to it is to change the line-heights of the higher-level headings like the h1, h2 and h3 heading, as they tend to get a bit big at times. So, something like h1, h2, h3 {line-height: 1.1} should be good.
    • Another way you could center something in a container is by using something like display: grid; place-content: center to get the same result. You could also just as easily use display: flex and place-content: center on the parent element, it’s completely up to you. Also if this doesn’t give you the result you desire you could also try place-items instead of place-content, and if that doesn’t give you the results you want you could look into justify-content: center, and align-items: center.
    • I'd recommend naming your variables after their functions, not their specific names. This will allow you to re-use the same variables in different projects. Instead of writing something like --dark-blue, you’re better off pinpointing what the function of the colour is, whether it’s an accent colour (a colour used sparingly sort of like a highlight for certain elements of parts of elements), primary colour (main colours used in the UI) or a neutral colour (white, shades of gray black). Here is an article which goes into more depth: (https://m2.material.io/design/color/the-color-system.html#color-theme-creation) In doing so in the next project you can literally just copy and paste these variables and change the values because your project will more than likely have colours that function as primary, accent and neutral colours. The same can be done for typography. You could use the prefix fs for font-size and ff for font-family and fw for font. So, you could have something like fs-heading (for the heading font size), and you could do something like --ff-base and --ff-accent (for the base or regular font used on the site, and accent for things like headings), and finally something like --fw-light for lighter font-weights like 100, --fw-normal for font-weights like 400, --fw-bold for font weights like 700, etc. Writing your variables this way you also don't have to change their names from project to project all you have to really change is the value.
    • Also I'd recommend not using the hsl function in your color variables. You're better off just mentioning the values (i.e. '--soft-blue: 215, 51%, 70%' instead of '--soft-blue: hsl(215, 51%, 70%)'). This will allow you to later add an alpha value if needs be. For example, if you wanted the soft-blue colour to be slightly transparent, but used the variables the way you've written it, you wouldn't have that flexibility to do so. However, if you only used the values, then you could do something like this. 'color: hsl(var(--soft-blue), 50%)'

    Hope this helps:) And if you want me to explain anything a bit better just let me know, I was trying to keep this not too long, so I had to rush through some concepts. But like I said before, overall great work, and looking forward to seeing future builds.

    0
  • Richard Ogujawa 👨‍💻•170
    @RichardOgujawaPosted almost 2 years ago

    Hi Abdellhamid!

    Hope you're keeping well:) I really like how you've indented your HTML really makes it a lot easier to read, and I appreciate your usage of multiple h1s. I initially thought it was something that was to be avoided at all costs, but after reading some article have come to learn that it's actually a gray area. I'm someone who tends to err on the side of caution when it comes to these things though so for now I'll just stick with one h1 tag per page, but you definitely got me thinking and challenging what I believed to be true all this while which I like doing so thank you.

    However, I do have some suggestions for you. But these are just my opinion so feel free to take this on board or take it with a grain of salt, no hard feelings.

    In the HTML:

    • I'd recommend using a main tag instead of a div with a class of header, because you want to wrap your main content in a main tag as opposed to a div.
    • I'd recommend using an article tag for the stats preview component. The article tag is a tag used for components that can be easily understood without any information around it. In otherwords you can look at it without it being in the context of the full site and it ill still makes sense. This allows it to be easily reusable and distributable. For example, blog posts, articles, components, widgets, etc. More on it here: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/article
    • I'd recommend looking into BEM Naming Conventions for naming classes. It makes it easier for you to write your class names without having to think too much about it, and it also makes it easier for other developers to understand your classes, because the naming convention is quite a logical one. There are two articles I'd recommend reading on it, and once you read them you're pretty much set. The first one talks about how to write BEM (https://codeburst.io/understanding-css-bem-naming-convention-a8cca116d252 ) and the second one talks about common problems you may run into when writing BEM and how to overcome them (https://medium.com/fed-or-dead/battling-bem-5-common-problems-and-how-to-avoid-them-5bbd23dee319)
    • I'd recommend using a picture tag instead of just the image tag because it's a much better way to add responsive images to your websites instead of just relying on CSS. CSS simply scales up and scales down an image, so if you use only one image to cater for the fact that it would be on both larger screens and smaller ones, you'd have to use a high-resolution image in your src for the image tag. However, not all users need that high-resolution image, and would prefer to load a lower-resolution one because their device might be slow as is, or they may just be using a device with a smaller screen and therefore loading a high-resolution file would be overkill, because even though it's scaled down it's still a high-resolution file meaning it's a large file to load. More on all that here: (https://blog.bitsrc.io/why-you-should-use-picture-tag-instead-of-img-tag-b9841e86bf8b).
    • Icons and images (unless it's purely decorative) should have alt text attached to it. The only other time you may not want alt text on your icons is when it's beside text that already essentially describes what it is, such as an icon of a happy dog beside text that says 'happy dog' because if you include the alt text the screen reader would end up reading out "happy dog, happy dog.”
    • Also when writing your alt text write it as though you're describing the image on the screen to a blind friend so they feel like they too can see it.

    In the CSS:

    • Another way you could center something in a container is by using something like display: grid; place-content: center to get the same result. You could also just as easily use display: flex and place-content: center on the parent element, it’s completely up to you. Also if this doesn’t give you the result you desire you could also try place-items instead of place-content, and if that doesn’t give you the results you want you could look into justify-content: center, and align-items: center.
    • I'd recommend using em and rem units. Em units for things like padding, margins, border, etc. and rem units for fonts. They are both linked to the font sizes you use so with this method you can easily adjust the size of a lot of things on your page simply by adjusting the font-size in the html selector. Also because em is linked to the font-size, you don't have to scale up buttons if you increase the font-size, it'll do so automatically.

    Hope this helps:) And if you want me to explain anything a bit better just let me know, I was trying to keep this not too long, so I had to rush through some concepts. But like I said before, overall great work, and looking forward to seeing future builds.

    0
  • Richard Ogujawa 👨‍💻•170
    @RichardOgujawaPosted almost 2 years ago

    Hi Lucas!

    Hope you're keeping well:) I really like your coding solution, especially the fact that you indented your code. I also like how you used an anchor tag for the button. I'd be tempted myself to use a button tag, but after doing some research into it it seems to be a pretty gray area, some people say you should never break away from semantic HTML and use an anchor tag in place of a button, and some would say that if the button results in a URL change, i.e. if it links to another document, then by all means wrap it in an anchor (for ex. here: https://www.ancestry.com/corporate/blog/buttons-vs-anchors) and style it like a button in CSS (for example in this article: https://accessibleweb.com/question-answer/is-it-ok-to-have-a-link-that-looks-like-a-button) I also appreciat the use of alt text, haven't really seen that being used in other solutions yet.

    However, I do have some suggestions for your code. Then again these are just my opinion so feel free to take this on board or take it with a grain of salt, no hard feelings.

    In the HTML:

    • The main tag is for the main content on your site, but it's to be used as a container for it as opposed to being used directly on the content. The order summary card is the main content on the site so the <main> tag should be wrapped around it as a container. For the order summary element itself I'd recommend using an article tag, as article tags are used for elements that can be understood on their own without any information around them for context.
    • I'd recommend looking into BEM Naming Conventions for naming classes. It makes it easier for you to write your class names without having to think too much about it, and it also makes it easier other developers to understand your classes, because the naming convention is quite a logical one. There are two articles I'd recommend reading on it, and once you read them you're pretty much set. The first one talks about how to write BEM (https://codeburst.io/understanding-css-bem-naming-convention-a8cca116d252 ) and the second one talks about common problems you may run into when writing BEM and how to overcome them (https://medium.com/fed-or-dead/battling-bem-5-common-problems-and-how-to-avoid-them-5bbd23dee319 )
    • I'd recommend using the picture tag, it's better than just using the img tag. More on that here: https://blog.bitsrc.io/why-you-should-use-picture-tag-instead-of-img-tag-b9841e86bf8b

    In the CSS:

    • I'd recommend using :focus instead of :active alongside the :hover pseudo-selector, because the :hover and :focus do the same thing. This is when a user is either hovering over the element with a mouse or they have used the tab key on their keyboard to tab to it. In other words both of them refer to the moment(s) before the interaction with the element, the :active selector on the other hand is when the user has actually clicked on the element, and is therefore interacting with it.
    • A faster way to write two pseudo-selectors like :hover and :focus together without doing .btn:hover, :btn:focus, is .btn:is(:focus, :hover). The :is pseudo-selector is a relatively new selector, which allows you to select multiple selectors. It does the same thing as another pseudo-selector called :where() the only difference is that :where() is easily overriden because it has a specificity of zero, and is takes the specificity of the selector in its list with the highest specificity. That's why where is typically left for situations when you want to easily override what you're about to write in the CSS rule, for example in a CSS reset. More on those two pseudo-selectors here: https://developer.mozilla.org/en-US/docs/Web/CSS/:where
    • An easier way to center content in a container would be to use {display: grid; place-content: center;} on the container element.

    Hope this helps:) And if you want me to explain anything a bit better just let me know, I was trying to keep this not too long, so I had to rush through some concepts.

    0
  • Richard Ogujawa 👨‍💻•170
    @RichardOgujawaPosted almost 2 years ago

    Hi there,

    Hope you're keeping well:) I really like your coding solution, especially the fact that you indented your code. It makes it easy to read. I appreciate that you used the different images instead of just scaling it down for the mobile version, didn't see many other people doing that, unless I wasn't looking well enough.

    However, I would make some changes to the code. Then again these are just my opinion so feel free to take this on board or take it with a grain of salt, I wish you all the best either way.

    In the HTML:

    • I wouldn't recommend using a line break (<br>) after your paragraph. It's a block element so it will take up the full width of its parent container anywhere and forces any content beside it in the HTML onto a new line. If you were using it to create a space between the paragraph text and the heading, I also wouldn't recommend it because you could easily achieve that with a margin-bottom in CSS which gives you more control of the height of the space between the two elements. Actually you could size the height of the br by selecting it in the CSS, but that just results in more lines of code than you need honestly.
    • I'd recommend looking into BEM Naming Conventions for naming classes. It makes it easier for you to write your class names without having to think too much about it, and it also makes it easier other developers to understand your classes, because the naming convention is quite a logical one. There are two articles I'd recommend reading on it, and once you read them you're pretty much set. The first one talks about how to write BEM (https://codeburst.io/understanding-css-bem-naming-convention-a8cca116d252 ) and the second one talks about common problems you may run into when writing BEM and how to overcome them (https://medium.com/fed-or-dead/battling-bem-5-common-problems-and-how-to-avoid-them-5bbd23dee319 )
    • A product image is part of the content of the site, it shows the audience what the product their going to be buying looks like, in otherwords it's not there for decoration, so I wouldn't recommend using the background-image: url() method to display the image, because you can't include any alt text when you use that method.
    • If you want to change your images out at smaller screen sizes, like I saw you did in your CSS, I'd recommend doing this in the HTML using a picture tag. https://blog.bitsrc.io/why-you-should-use-picture-tag-instead-of-img-tag-b9841e86bf8b
    • Also a product card is a piece of content that can be easily understood without any other content around it so instead of just using a div for it, I'd recommend using an article tag. More on that here: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/article
    • The prices aren't really a list of anything, so I wouldn't recommend using an unordered list for it.
    • If you want a space between the Add to Card text and the icon I wouldn't recommend using   because once again you can't control the size of that. Instead I'd recommend just leaving that to when you get to the CSS. You could do something like wrap the Add to Cart in a span, and then simply do a display:flex on the button, and then add a gap between the two flex-items inside (the icon and the button text)
    • Icons should have alt text unless the text beside them is pretty much the same as what you would put in the alt text, in that case, it becomes redundant to include alt text because the screen reader would simply be reading out the same text twice.

    In the CSS:

    • An easier way to center content in a container would be to use {display: grid; place-content: center;}
    • I'd recommend using em and rem units. Em units for things like padding, margins, border, etc. and rem units for fonts. They are both linked to the font sizes you use so with this method you can easily adjust the size of a lot of things on your page simply by adjusting the font-size in the html selector. Also because em is linked to the font-size, you don't have to scale up buttons if you increase the font-size, it'll do so automatically.

    Hope this helps:) And if you want me to explain anything a bit better just let me know, I was trying to keep this not too long, so I had to rush through some concepts.

    0
  • Richard Ogujawa 👨‍💻•170
    @RichardOgujawaPosted almost 2 years ago

    Hi Arsenije,

    Hope you're keeping well:) I really like your coding solution, especially the fact that you indented your code. It makes it easy to read. I appreciate how you used comments to write headings in your CSS to make it more easily navigable.

    However, I would make some changes to the code. Then again these are just my opinion so feel free to take this on board or take it with a grain of salt, I wish you all the best either way.

    In the HTML:

    • I'd recommend wrapping the main content of your project with a main tag, even if it's the only element on the page.
    • Check out BEM Naming convention to make naming classes easier and also to make it easier for other developers to understand your code better.
    • Heading levels are used for the most to least important text on the page, since that's the case I'd recommend having the 'perfume' text as a h2 because it can be considered as a category tag, and leave the price as spans or paragraph text.
    • I'd recommend using an article tag for the product card as a whole, because it would be regarded as a piece of content that can be understood without any information around it, which is all an article really is in HTML (https://developer.mozilla.org/en-US/docs/Web/HTML/Element/article)
    • It isn't the best idea to use the <s> tag to put a line through your text because it results in that bit of text not getting read out by screen reader software. More on that here: https://www.tempertemper.net/blog/be-careful-with-strikethrough Instead, you could just wrap that bit of text in a span and do something in the CSS like text-decoration: line-through (https://www.w3schools.com/cssref/pr_text_text-decoration.php)
    • I wouldn't recommend using class="heading" for a heading tag. If you want to describe the heading tag better I'd recommend something like class="title" or class="subtitle", etc.
    • When it comes to HTML you want to make it as meaningful and descriptive as possible. If something is a button you want to use a button tag, if something is a paragraph use a p tag, etc.
    • I'd recommend using a picture instead of just the image tag because it's a much better way to add responsive images to your websites instead of just relying on CSS. CSS simply scales up and scales down an image, so if you use only one image to cater for the fact that it would be on both larger screens and smaller ones, you'd have to use a high-resolution image in your src for the image tag. However, not all users need that high-resolution image, and would prefer to load a lower-resolution one because their device might be slow as is, or they may just be using a device with a smaller screen and therefore loading a high-resolution file would be overkill, because even though it's scaled down it's still a high-resolution file meaning it's a large file to load. More on all that here: (https://blog.bitsrc.io/why-you-should-use-picture-tag-instead-of-img-tag-b9841e86bf8b).
    • Also when it comes to heading levels, they should only ever decrease by one at a time, so you shouldn't ever really have a page with say a h1, h3 and h4. If you only have three headings levels on a page, it should go down by one level at a time, i.e. h1, h2, h3. Because at the end of the day heading levels are used for ranking headings from the most to least important on the page.
    • Icons and image (unless it's purely decorative) should have an alt text attached to it. The only other time you may not want an alt text on your icons is when it's beside text that essentially describes what it is, such as an icon of a happy dog beside beside text that says 'happy dog' because if you include the alt text the screen reader would end up reading out "happy dog, happy dog."

    In the CSS:

    • An easier way to center content in a container would be to use {display: grid; place-content: center;}

    Hope this helps:) And if you want me to explain anything a bit better just let me know, I was trying to keep this not too long, so I had to rush through some concepts.

    Marked as helpful
    1
  • Richard Ogujawa 👨‍💻•170
    @RichardOgujawaPosted almost 2 years ago

    Hi Acezalba,

    Hope you're keeping well:) I really like your coding solution, and like how you've incorporated semantic tags in your HTML. How you used radio buttons for your rating buttons 1 to 5, and how you used a fieldset, because none of the other submissions I've seen have done that so far. In the CSS I also like how you used :where instead of the :is pseudo-selectors, because of it's zero specificity making it easy to override with your custom CSS.

    However, I would make some changes to the code. Then again these are just my opinion so feel free to take this on board or take it with a grain of salt, I wish you all the best either way.

    In the HTML:

    • I'd recommend looking into BEM Naming Conventions for naming classes. It makes it easier for you to write your class names without having to think too much about it, and it also makes it easier other developers to understand your class names, because the naming convention is quite a logical one. There are two articles I'd recommend reading on it, and once you read them you're pretty much set. The first one talks about how to write using the BEM Naming Convention (https://codeburst.io/understanding-css-bem-naming-convention-a8cca116d252) and the second one talks about common problems you may run into when writing BEM and how to overcome them (https://medium.com/fed-or-dead/battling-bem-5-common-problems-and-how-to-avoid-them-5bbd23dee319)

    In the CSS:

    • I would recommend grouping your CSS properties into categories, for example sizing properties (padding, margin, width, height, border, etc.) typography-related properties (font-size, line-height, etc.), display properties (flex, grid, flex-direction, grid-template-rows, etc.), positional properties (position, top, left, etc.) appearance properties (color, opacity etc.). And by grouping them I simply mean having an empty line between them.
    • If you want to center content in a container I'd recommend doing something like {display:grid; place-content: center}. If this doesn't give you the desired result you could also try place-items:center instead of place-content: center. Here's a video on the two and the difference between them: https://www.youtube.com/watch?v=vNwoDkn7AIc
    • You could use variables to set your typography settings too. You could use the prefix fs for font-size, ff for font-family, and fw for font. So, you could have something like --fs-heading (for the heading font size), you could do something like --ff-base and --ff-accent (--ff-base for the base or regular font family used on the site i.e. paragraph text, links, button text, etc., and --ff-accent for the font-family for headings), and finally something like --fw-light for a thin font weight for example a font weight of 100, --fw-normal for font weights like 400, and --fw-bold for font weights like 700, etc. If you want to go for higher font weight you could do something like --fw-bolder, or --fw-black. Writing your variables this way will mean that you don't have to change the variable name from project to project all you have to really change is the value.
    • Similarly, instead of writing something like --dark-blue, you’re better off pinpointing what the function of the colour is, whether it’s an accent colour (a colour used sparingly sort to highlight things on the page), a primary colour (the main colour used in the UI) or a neutral colour (white, shades of gray and black). You can also have different shades and tints of accent and primary colours too, and in that case you could do something like --col-primary-100 for a light version of the primary colours, --col-primary-400 for the primary colours in its normal state, and --col-primary-900 for a dark version of the primary colour. Here is an article that goes into more depth on what an accent or primary colour is: https://m2.material.io/design/color/the-color-system.html#color-theme-creation) Writing your colours variables like this also mean you don't have to change them from project to project, because the names aren't linked to the colours you use but rather their function.
    • I'd recommend not using the hsl function in your color variables. You're better off just mentioning the values (i.e. '--soft-blue: 215, 51%, 70%' instead of '--soft-blue: hsl(215, 51%, 70%)'). This will allow you to later add an alpha value if needs be. For example, if you wanted the soft-blue colour to have an opacity of 50%, but used the variables the way you've written it you wouldn't have that flexibility to do so. However, if you only used the values, then you could do something like this. 'color: hsl(var(--soft-blue), 50%)'
    • I'd recommend using utility classes for things that are repetitive, like display: flex, this reduces the amount of CSS you have to write. For example, instead of writing display: flex in each CSS rule, you could just make a utility class called flex-group, or flex, and have display flex in that CSS rule, and then simply add that class to any of the HTML elements that are either 'display: flex'.
    • I'd recommend grouping your CSS and giving them headings to make them more easily navigable for yourself and for other developers too using comments. For example you could have a heading like /CSS Reset/ and then having all your CSS reset rules under it, and then /Global Stylings/ and having all your global stylings there for h1, p, link stylings that are custom to that project, and then /Custom CSS/ which are more specific CSS rules like in your case for selectors like .rating-card button, or .rating-card p, etc.

    Hope this helps:)

    0
  • Richard Ogujawa 👨‍💻•170
    @RichardOgujawaPosted almost 2 years ago

    Hey Kaysiwill,

    Hope you're keeping well. Really like the code you've written man. Just have one minor tweak I would recommend which I learned from one of Kevin Powell's videos which I think would benefit you too.

    I'd recommend not using the hsl function in your color variables. You're better off just using the values (i.e. '--soft-blue: 215, 51%, 70%' instead of '--soft-blue: hsl(215, 51%, 70%)'). This will allow you to later add an alpha value if needs be. For example, if you wanted the soft-blue colour to be slightly transparent, but used the variables the way you've written it, you wouldn't have that flexibility to do so. However, if you only used the values, then you could do something like this. 'color: hsl(var(--soft-blue), 50%)'

    Hope this helps:)

    0
  • Richard Ogujawa 👨‍💻•170
    @RichardOgujawaPosted almost 2 years ago

    Hi Ragilbuaj,

    Hope you're keeping well man. Great job with the final product however, I to answer your question I would recommend using different HTML tags to allow the browser to better understand your site, it would also make your site more easily navigable for a screen reader. For example instead of using a div for the card I would recommend using an article (https://developer.mozilla.org/en-US/docs/Web/HTML/Element/article), I would recommend using a picture tag instead of a div to wrap around the img (https://blog.bitsrc.io/why-you-should-use-picture-tag-instead-of-img-tag-b9841e86bf8b), and finally I would also recommend using heading and paragraphs tags instead of divs.

    I re-wrote how I think it would be written using semantic HTML, and sent you a pull request. Check it out and let me know what you think. I didn't change any of the class names so you shouldn't have to rewrite any of the CSS:)

    Marked as helpful
    0
  • Carlos•110
    @carlosmndzgSubmitted almost 2 years ago

    Is there a better way to change the window from "rating" to "thanks" than using a CSS class that makes display: none to elements? It was difficult to make the buttons for the rating, is there a better implementation?

    If you have any tips I would be grateful!

    Thank you in advance!

    Responsive Interactive Rating Component

    1
    Richard Ogujawa 👨‍💻•170
    @RichardOgujawaPosted almost 2 years ago

    Hi Carlos!

    Great job with the code, love what you've done man. Just had a few recommendations but feel free to disagree.

    In the HTML:

    • To write more semantic HTML, I would recommend using an article tag instead of a div for the rating component because it's a self-contained composition as it doesn't require other information around it to be understood. More on HTML articles here: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/article
    • I appreciate the use of BEM man
    • I know that more than likely the heading in this component wouldn't be a h1 level heading if it was included in a website, but given that it's the only content on the page it should be regarded as the main heading on the page, and therefore does deserve to be written using a h1. The h1 is simply for the most important text on the page, the introduction, and what is the page about. Every page should have one. h2, h3, etc. are for headings of lesser importance, and for them to be of lesser importance, there must first be a heading of greater importance. If the h1 tag appears to be inappropriate it's probably because of its appearance, but don't let appearance prevent you from making your HTML as semantic as possible. You can always fix the appearance later with the CSS.
    • I'd recommend using the picture tag, it's better than just using the img tag. More on that here: https://blog.bitsrc.io/why-you-should-use-picture-tag-instead-of-img-tag-b9841e86bf8b

    In the CSS:

    • I'd recommend not using the hsl function in your color variables. You're better off just mentioning the values (i.e. '--soft-blue: 215, 51%, 70%' instead of '--soft-blue: hsl(215, 51%, 70%)'). This will allow you to later add an alpha value if needs be. For example, if you wanted the soft-blue colour to be slightly transparent, but used the variables the way you've written it, you wouldn't have that flexibility to do so. However, if you only used the values, then you could do something like this. 'color: hsl(var(--soft-blue), 50%)'
    • I'd recommend separating the different properties in your CSS based on what they do, for example having display properties separate from the dimension properties separate from the color properties, etc. And by separate I simply meaning grouping them together with a space between each group. It makes the code a lot easier to read. However, I would recommend putting anything that has to do with the dimension of the elements in the same group though, for example, instead of separating width from padding, I'd put them in the same group, because they all contribute to the overall size of the element. Things like margins, and borders also contribute to the overall size of an element. I also include font-size when I write my CSS, but it's a bit more debatable I guess.
    • I like how you used radio inputs instead of buttons, it was something that I had in mind to do but wasn't able to execute at the time and so just settled on doing buttons. But, after seeing what you did I decided to go back and give it a try again and got it so thank you! I would also recommend using a fieldset around the radio buttons to group them together and a legend with an sr-only class on the fieldset too.

    Hope this helps, and sorry for any typos if there are any:)

    Marked as helpful
    1
  • Alan Torino•60
    @Alan08tSubmitted almost 2 years ago

    Hello everyone!

    In this, my first project presented on this platform, I have decided to choose the first project that the platform provides us with.

    So while the design is very simple, that doesn't stop me from practicing my flexbox skills, I'm very surprised at how much we can save on code with more practice.

    It's amazing!

    Well, for me it is a pleasure if you, dear community of this beautiful profession, can give me your opinion to learn more and more!! Thanks for reading me and my horrible English 😅😅.

    Simple Flexbox solution of this QR project!

    2
    Richard Ogujawa 👨‍💻•170
    @RichardOgujawaPosted almost 2 years ago

    Hi Alan!

    Great job with the code, love what you've done man. Just had a few recommendations but feel free to disagree.

    In the HTML:

    • Get into the practice of wrapping the main content of your project with the main tag, even if it's the only piece of content on the page.
    • I know that more than likely the heading in this component wouldn't be a h1 level heading if it was included in a website, but given that it's the only content on the page it should be regarded as the main heading on the page, and therefore does deserve to be written using a h1. The h1 is simply for the most important text on the page, the introduction, and what is the page about. Every page should have one. h2, h3, etc. are for headings of lesser importance, and for them to be of lesser importance, there must first be a heading of greater importance. If the h1 tag appears to be inappropriate it's probably because of its appearance, but don't let appearance prevent you from making your HTML as semantic as possible. You can always fix the appearance later with the CSS.
    • I'd recommend looking into the BEM Naming Convention for classes. It makes it easier to write classes for you as a developer, and easier for other developers to understand your code (https://codeburst.io/understanding-css-bem-naming-convention-a8cca116d252)
    • I'd recommend using the picture tag, it's better than just using the img tag. More on that here: https://blog.bitsrc.io/why-you-should-use-picture-tag-instead-of-img-tag-b9841e86bf8b

    In the CSS:

    • I like how you've separated the different properties in your CSS based on what they do, for example how you have the display properties separate from the dimension properties separate from the color properties, etc. It makes the code a lot easier to read. However, I would recommend putting anything that has to do with the dimension of the elements in the same group though, for example, instead of separating width from padding, I'd put them in the same group, because they all contribute to the overall size of the element. Things like a margins, and borders also contribute to the overall size o an element. I also include font-size when I write my CSS, but it's a bit more debatable I guess.
    • I'd recommend using em and rem units. Em units for things like padding, margins, border, etc. and rem units for fonts. They are both linked to the font sizes you use so with this method you can easily adjust the size of a lot of things on your page simply by adjusting the font-size in the html selector. Also because em is linked to the font-size, you don't have to scale up buttons if you increase the font-size, it'll do so automatically.

    Hope this helps, and sorry for any typos if there are any:)

    Marked as helpful
    1