Maryam Ali• 220
@Maryamhussein
Submitted
Christ Kevin Touga Watat
@Christ-Kevin
All comments
@Christ-Kevin
Posted
Hello,
nice challenge. To center your body horizontally it's quite easy. remove the height you wrote and use "min-height: 100vh" for your body. It prevents the used value of the height property from becoming smaller than the value specified for min-height.
Happy Coding
Christ
Marked as helpful
0- MisiaczekOOO• 50
@MisiaczekOOO
Submitted
@Christ-Kevin
Posted
Hello, to avoid the Error "Image must have alternate text" you have to add the alt attribute in your image tag. This link(https://www.w3.org/WAI/tutorials/images/decision-tree/) helps you to decide what alt you can use. I also noticed that your image does not change on hover as it have to. To do this you can put your image inside a div container and then use another div container that will contain the eye(The eye image have to be added via css with the background image css property ) that will appear on the hover state.
Chech my solution here(https://github.com/Christ-Kevin/NFTchallenge/tree/main) to get more infos
Keep coding
Christ
0 - Jan Toma• 380
@jantoma
Submitted
@Christ-Kevin
Posted
Hello congrats on uploading your solution.
my suggestion is that you should use only one main tag as the child of the body to get rid of the warning "All page content should be contained by landmarks". I also noticed that you forgot the hover state on Equilibrium and Jules Wyvern. But in general your work is great.
Keep coding
Christ
0 - Pedro• 130
@pdroaq
Submitted
@Christ-Kevin
Posted
Hi @Pedro,
you have a nice looking challenge and I like how you use the transitions on your rating buttons. The only Issue that I see in your challenge is that a user can press on submit and get the thankyou message even if he has'nt clicked on a rating button.
A solution would be to disabled the submit button, when the user has not yet clicked on a button, or to display an alert message to the user when he click on submit althought he has not yet clicked on a rating button.
I hope this help.
Happy coding :)
Christ
0 - Arturo Muñoz• 170
@arturo0427
Submitted
recommendations?
@Christ-Kevin
Posted
Hi @arturo0427, congrats on uploading this solution. I like how you used BEM in your HTML, and i'm impressed cause I never knew that it's possible to add an event listener to the container. I was always thinking that I should add the event listener to all the items using .forEach. I also like how you used your if ... else to avoid that the user press the submit button and a get an empty result, when he has not yet selected a rating.
The only recommendations that I can make is that to solve your accessibility issue you have to add at least one H1 tag in your html file "How did we do?" is a good candidate that can fit in one h1, but you would have to replace your sections in divs cause sections dont allow h2 as children. And you would've to put your two divs inside one main tag.
I really hope, this learn
Happy coding
Christ
Marked as helpful
0 - Guilherme• 180
@lheerme
Submitted
@Christ-Kevin
Posted
hi @guifrangolino,
i really like how you did this challenge. Your javascript file is the simplest one I've seen sofar concerning this challenge. I notice in your solution that the user is able to submit an empty "" rating even if he does not choose between 1, 2, 3 ... Would not it be a great idea if the using could submit his rating only after selecting one ? I'm just asking :). I mean that the button can be disabled until the user select a rating and then enabled after the rating has been selected
Nice job
Christ
Marked as helpful
0 - Vinson• 90
@cryptovinzon
Submitted
@Christ-Kevin
Posted
Hey @cryptovinzon,
congrats for uploading this nice solution. I appreciate how you hide a part of the index document and I think screen-readers are also not able to read this second part of the index.html untill the rating is submitted. Once the user submitted his rating, he is also able to reload the web page and submit his rating one more time ;) The suggestion that I have is that you should try to use semantic elements like "header", "main", "article", "aside" or "footer" and you should reduce the use of "div" tags in your html file. The use of semantic elements would improve the accessibility, the readability and the concistency of your web page. Check out this Webpage for more infos(https://dev.to/kenbellows/stop-using-so-many-divs-an-intro-to-semantic-html-3i9i).
I really hope it helps
Happy coding :)
Christ
0 - Yunus Emre Çınar• 360
@Yunuscinar41
Submitted
What I add extra from the request ?
- I add a linear gradient to the background
@Christ-Kevin
Posted
congrats for your solution @Yunuscinar41, to correct the html issue you should remove the charset attribute on the script tag. I also appreciate your "rate again" button. The user have this opportunity to rate a second time if he wants or if he changes his mine. But there is a problem cause the user can click the submit button even if he has not given a rating and he get the default rating "5" as a result. I would suggest you to use the "localStorage.setItem()" method to save the textContent in your rating-event inside the browser server and then use the "localStorage.getItem()" to get this content that would be written on the page that loads after pressing the submit button.
I really hope It helps
Happy coding :)
Marked as helpful
0 I'm pretty happy with this. I normally build apps, so this was a good opportunity to experiment more with fluid typography and intrinsic design.
I'm not 100% happy about having to specify the grid column widths for the large viewport. I would've liked it that the image took up enough space as it needed to maintain its aspect ration, while the content section expanded in width as much as it needed. But I couldn't get that to work. The
repeat(2, 1fr)grid seemed like an acceptable compromise.@Christ-Kevin
Posted
Hi Alex,
congratulation. you have a nice looking website. I would like to give you some ideas how you can avoid the html validation issues. I think your website can look better if you define the width and height of your images in the css and not in the html file. While defining the width and the height in your css you can use min and max values if the image size increase too fast with the viewport compare to the container size.
Another issue is the alignment of your second price. it should have the same alignment as the first one. My suggestion would be to use the " " to create space between the first price and the second one, then you should use another span for the second price. And to make sure that the second price is vertically in the middle of the container you can make " display: inline-flex; align-items: center;" for your spans.
I hope this could help you
Happy coding
Christ
Marked as helpful
0- David• 670
@cutch14
Submitted
@Christ-Kevin
Posted
I like your code, congratulations.
I especially like your shorthand to define the border radius. I never used it and I think I'll from now. However your code has an accessibility issue and this is due to the fact that you have a p tag inside a button tag. To avoid this kind of problem, you should use a link( or a) tag instead of the button tag. The following link(https://css-tricks.com/buttons-vs-links/) can tell you why it's better to avoid buttons in a project like this. In this other link(https://css-tricks.com/a-complete-guide-to-links-and-buttons/) you'll learn when you can use buttons or when you can use links
Happy coding
Christ
0 @Christ-Kevin
Submitted
@Christ-Kevin
Posted
@pjooklas thanks, i just removed the dashed border around and made it a bit smaller
0
