@ArsenijeDragic
Submitted
@RichardOgujawa
@ArsenijeDragic
Submitted
@RichardOgujawa
Posted
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:
Border-radius problem
Minor changes
As always, if you want any help going over anything I said, just let me know:) Happy coding!
Marked as helpful
@Teles23
Submitted
@RichardOgujawa
Posted
@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:)
@Kaysiwll
Submitted
@RichardOgujawa
Posted
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:
In the CSS:
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.
@Boddaa
Submitted
@RichardOgujawa
Posted
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:
In the CSS:
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.
@lucasres196807
Submitted
Another challenge completed successfully.
@RichardOgujawa
Posted
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:
In the CSS:
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.
@Teles23
Submitted
@RichardOgujawa
Posted
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:
In the CSS:
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.
@ArsenijeDragic
Submitted
@RichardOgujawa
Posted
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:
In the CSS:
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
@acezalba
Submitted
@RichardOgujawa
Posted
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:
In the CSS:
Hope this helps:)
@Kaysiwll
Submitted
@RichardOgujawa
Posted
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:)
@ragilbuaj
Submitted
I'm not sure using div for the content of the card is the right thing to do
@RichardOgujawa
Posted
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
@carlosmndzg
Submitted
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!
@RichardOgujawa
Posted
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:
In the CSS:
Hope this helps, and sorry for any typos if there are any:)
Marked as helpful
@Alan08t
Submitted
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 😅😅.
@RichardOgujawa
Posted
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:
In the CSS:
Hope this helps, and sorry for any typos if there are any:)
Marked as helpful