kohicha• 340
@kohicha
Submitted
Hello everyone! I am pretty much a beginner still when it comes to javascript since I learned it and never really did any projects with it yet. I'd like some criticism about my JavaScript code!
George Daris
@GeorgeDaris
All comments
- George Daris• 60
@GeorgeDaris
Posted
Hey there, Kohicha!
Your solution looks good for the most part. When it comes to your JavaScript, I would suggest you use more specific naming, such as "numPeopleInput" to differentiate variables from each other. You have two "total amount" variables, one for the DOM node and one for the numerical value of the actual amount, which could get confusing in a larger project because it isn't easy to tell them apart.
Instead of using a keydown event for the inputs you could look into using a change event, although it wouldn't make too big of a difference. Just keep it in mind for other projects where it could become useful.
When the user inputs the amount of people, a "Can't be zero message" appears even when the value is greater than zero. Can you find out why it happens? It disappears the moment another number gets added. Another thing to look out for is the fact that the total amounts display "NaN" after a reset.
Aside from what I mentioned above your JS did what it was supposed to, and you did I good job at changing the styling of the reset button with it, depending on whether all the fields had been populated or not.
There are some improvements you could make to your HTML and CSS. First of all, use semantic labels for inputs, so screen reader users can understand what inputs are for. Secondly, you could remove the set width of 11rem you have set on the percentage inputs to let them flex depending on the size of the container. Finally, you could make some changes to your media query break points to serve a mobile view a little earlier.
I hope these suggestions help you out! Continue coding and learning, and if you ever need help feel free to ask. MDN's Web Docs are also worth looking into if you haven't already.
0 - Mustafa• 100
@SmokerSacred
Submitted
This was the first time learning and using an API . The only place I had a hard time with was the box shadow on hover where the shadow on touchscreen devices stick, I still dont know the fix
George Daris• 60@GeorgeDaris
Posted
Hello again, Mustafa!
Your solution is very close to the design, and your CSS is concise, as is your JavaScript code. Regarding the hover state persisting on mobile, I would suggest you take a look at this stack overflow solution which uses a media query to check whether the device uses a mouse or not to conditionally add the styling.
There are some improvements you could make to your HTML markup. Wrap your content inside a
maintag, and since the advice component is self-contained, you could use anarticletag instead. Theh1andpare used correctly, but your button isn't accessible. Instead of using a div with a nested image, use a nativebuttonelement and add the image as a pseudo element, or nest it without forgetting to specify it as a decorative element. You should also add a title attribute to it so screen reader users can know what it does.Hope this helps! You're on a good track!
0 - Mustafa• 100
@SmokerSacred
Submitted
Couldnt figure out where to put the icons
George Daris• 60@GeorgeDaris
Posted
One way of going about the icons is adding them as pseudo elements, since they are decorative elements. The input element doesn't accept pseudo classes, since it isn't a container element. To overcome this issue you can simply wrap it in a span, and using the ::before pseudo selector add the appropriate icons as background images.
Marked as helpful
1 - moonrose93• 370
@moonrose93
Submitted
I think its just fine?
George Daris• 60@GeorgeDaris
Posted
Looks great for the most part!
Your font has trouble getting loaded, so make sure to look into what is causing the error. I would suggest you change the break-point of your media query, as there is some overflow present at the 500px - 700px width range.
For the margin of your image left items, you could use a margin-right of
autoon the p tag. This will move the item to the far right no matter the size of the container.Hope this helps!
1 - Tanu• 80
@tanu-12
Submitted
This isn't complete just want some helpful feedbacks
George Daris• 60@GeorgeDaris
Posted
Hey, there!
You have made quite some progress from what I can see. The data from the form gets displayed on the cards as intended, and you have placed a check to make sure the user fills out the form before being able to submit it.
Here are some things you could consider to improve your app:
- Use relative units, such as rems, to make your design more responsive.
- Your cards are inside divs which take up more space than the cards themselves, making the form inaccessible when the width of the screen becomes smaller.
- Instead of using absolute positioning for the cards, consider placing them inside a grid container and using gaps or margins to create the spacing you are looking for. This can help make it work for mobile devices as well, where you would have a one column grid with two rows: one for the cards, and another for the form.
- The large top margin you have set on the form could be replaced by justifying the items of your container in the center. No matter how large or small the screen, your content will be placed in the middle vertically.
I hope this helps you consider some solutions to your problems. I would highly suggest you read articles from MDN Web Docs, as they cover a wide range of topics related to CSS and web development in general.
Happy coding!
Marked as helpful
1 - George• 790
@georgeCx5
Submitted
My first intermediate challenge. I really enjoyed making it 😄
Any feedback is kindly welcome! 🙂
George Daris• 60@GeorgeDaris
Posted
Hey there, George!
Your solution looks great, although I would suggest you change the breakpoints of your media queries, as I was served a mobile view on a laptop screen.
As for your code, I would highly suggest you try out the composition API as well and use it on a future project. I can't view your tailwind config file, but from your code I can tell that you used a lot of arbitrary values for your Tailwind classes, which could get simplified by extending the theme, which is similar to using custom properties when working with regular CSS.
Hope this helps!
0
