@Fareed-Ahmad7
Vishal
@vb8146649All comments
- @vb8146649
!!!This isn't properly structured.!!!
- use landmark tag such as
main,footer,header. i.e the card should be inside a main . - dont use
brunnecessarily , like you did in both heading and p . Make it flow naturally . - use
h2or lower heading since when integrating this component this wont be the main heading of a page , hence it should be h2 or lower. - use
text-align:centerin heading also . - I see the problem , you used
width:fit-contentin styling the body of the component thats why you have usedbrtag to mitigate the disaster . Its a bad practice dont use it like this. instead use amax-widthstyling that should be in rem or em , using px or vh or vw is never generally recommended . - No need to use
flexsince the tags are block and all will be displayed one after another. Usingflexunnecessarily can only lead to disaster . - Accessibility : the image tag should have a well defined description , not like
alt="image"when the image is important like a qr.
- use landmark tag such as
- @NavidSadat@vb8146649
- Use LandMark i.e
maintag inbody. - Give alt attribute here , It conveys a important thing in the component .
- Use width in rem rather than px in
.containerclass .
Rest All is Good , Great Work!!!
Marked as helpful - Use LandMark i.e
- @shaunthom@vb8146649
This is very badly done .
- I recommend you to try to make it with just html css and js , no need to use react , it will just complicate the thing .
- Styling is bad . Needs improvement.
- @NavidSadat@vb8146649
This is Good
- Good HTMl structuring , very easy to read .
- Dont give alt to decorative images ,
alttag is used to give description of something useful , this image doesnt convey anything useful . - I understand that you want to give custom cursor to the
.buttonclass but it doesnt have any interactivity , this may confuse the user .
Rest All Looks Good Great Work
Marked as helpful - @PattykevWhat are you most proud of, and what would you do differently next time?
I do my best to respect the design of the page.
What challenges did you encounter, and how did you overcome them?Represents the design of the non-numeroted and the numeroted list.
@vb8146649This is not correctly done !!!
- First you should use the colors given in the style file for background.
- Next the
<table>tag ofHTMLshould be used for containing the nutrition class in your code . <span> 0g</span>This is a very wrong practice try using css for styling this . It isnt good when reading the page through a screen reader .- Use
<footer>tag for landmark the attribution div element. - Its always a good practice to use a reset.css file to remove any inbuilt browser css style that will be applied to the webpage.
- You have used too many divs without any purpose . Always try to make the structure of the html easy to navigate through and understandable .
<div class="container"> <div class="container-elements">- Dont use px unit in max-width or padding always use rem or em since , many devices have different base font size and your site should be adaptive to these font sizes .
max-width: 1200px;- The Most Imp Thing site isnt responsive , the img doesnt take the full width of the screen in mobile devices .
- Use padding on the body tag to keep the container from touching the screen .
Marked as helpful - @CoolNight99What are you most proud of, and what would you do differently next time?
I think I got the functionality correct but the design is slightly inaccurate to what was given despite my many attempts to fix it.
What challenges did you encounter, and how did you overcome them?Besides the design, I mainly encountered challenges with getting the active states for the rating buttons to work properly but I was able to do it with some trial and error.
What specific areas of your project would you like help with?How to make the design match the given design.
@vb8146649Its nicely done , Only thing that I could think of that could make it better would be to use radio inputs , since you can choose only one of them and it is also better understandable . It would also make your code better readable for people with disabilities . Other than that all looks good .
- @alexpeteronojaWhat are you most proud of, and what would you do differently next time?
I am proud that I was able to complete this challenge.
What challenges did you encounter, and how did you overcome them?The share section was a bit of a hassle but I was able to get it.
What specific areas of your project would you like help with?Please to through the code and let me know how I can improve or best practices.
@vb8146649- This is not responding correctly to change in width and height in mobile size , I think you have used padding relative to the size of the screen .
- Component isnt centered in mobile view .
- Button doesnt change color when clicked .
- Accessibility vise its not looking good , think of like this , someone who is using screen reader whouldnt be able to tell what button or link he is clicking since there is no mentioning of it in the image alt .
use image alt when wrapping it in anchor tag
<a>.
- @AravindM817What specific areas of your project would you like help with?
Any feedback is appreciated.
@vb8146649Brilliant , I didnt thought that using grid-template areas would make it so easy . Honestly Its very good , I tried doing in tailwind and it just isnt efficient enough . Well Done !
- @ChristoDiZWhat are you most proud of, and what would you do differently next time?
Funciono desktop y mobile, me costo demasiado, gracias a tailwind por su documentacion. Flex lo mejor.
What challenges did you encounter, and how did you overcome them?Al principio me costo jugar con los componentes para poder adaptarlos con la resolución del dispositivo, toda la documentacion de Tailwind vale oro.
What specific areas of your project would you like help with?Me gustaria que los mas experimentados me indique si esta bien mi propuesta al tema responsivo, ya que lo hice solo viendo la documentacion de Tailwind. saludos
@vb8146649- First of all , Use the given colors and font in style file .
- Center the four component cards in the middle of the page using flex box .
- Use correct margins in rem or em , not in pixels , since rem and em are proportional to font-size , and many people use or change the default font-size for there ease .
- the card clips out of the screen when reducing the width after a certain point .
- you have applied margin-left on the mobile screen which causes the entire card to clip out of screen .
- You can check out my solution for the same challenge for reference or anybody else .
- @Otep02What challenges did you encounter, and how did you overcome them?
The difficulties of this challenge for me is how fit the image inside the box and make it responsive. But by making it as background, replacing and aligning of image becomes easy.
@vb8146649This looks good , but the site isnt responsive and clips after certain width limit is reached and dont use margin in component else you should use padding for the body tag . Use <s> </s> tag to strike-through a text ,it may look the same but the screen readers and other accessibility software wouldn't announce the price is strike-through and will confuse the buyers as to why there are 2 prices .
Marked as helpful - @Davickyz@vb8146649
Have you tried to do this without using js , I know its absurd but doing it with HTML is pretty fun .
