Ayush Nath
@Beats-AyushAll comments
- @Gabocz@Beats-Ayush
Nice job completing this challenge. There are some places where you could improve -
- The hover state isn't persisting on the normal buttons.
- Also upon selecting the bill and tip percentage, the error for the number of people is automatically coming up even though I haven't touched the input yet.
You can have a look at my attempt to see what I meant.
Marked as helpful - @njorogejeff@Beats-Ayush
- You can center the card by
- Using
margin: 2.5rem auto;
. This is shorthand. 2.5rem is themargin-top
andmargin-bottom
. - Using absolute positioning.
position: absolute; left:50%; transform: translateX(-50%);
- I think the footnote is fine its place. No such biggie. If you don't want it wrapping, give it a
min-width
.
Search on stackOverflow and MDN for these queries. They have explanations in great detail.
Marked as helpful - @Jack-LP@Beats-Ayush
Great job completing this challenge. JS looks good to me. Some issues with the css-
- You can use multiple backgrounds with css.
- You can cut the overlapping box-shadow using clip-path.
Hope this helps.
Marked as helpful - @minhnhut170701@Beats-Ayush
Great job completing the challenge. Some issues -
border-radius
is missing at some places.- Use
background: no-repeat
andbackground-size: cover
to avoid repetition of background-image. - Try to make it a bit more responsive using media-queries.
- JS has some flaws. When I choose some reward and submit my pledge, the modal-overlay comes up again.
- Pledge with no reward doesn't have a pledge and continue button placed there.
Marked as helpful - @kirtymeena@Beats-Ayush
Nice job completing this challenge. Some issues:
Background-color
of body is missing.- The text is the buttons is leaning towards the top. You can fix that by
display: flex; align-items: center;
.
- @Beats-Ayush@Beats-Ayush
Damn like so many HTML Errors.
- @Gareth-Moore@Beats-Ayush
That's actually a very good extension. Thank you for sharing.
I just found a slight error which you may want to rectify. It's not significant but still caught my eye.In the mobile layout the border-radius is not being applied to the top-left and top-right corners of your main card. You can enforce that using
overflow: hidden
. Hope this helps.Marked as helpful - @Mister-Zeng@Beats-Ayush
Hello nice attempt at this challenge. Some points where there can be improvement -
- Instead of setting <input type='number' />, maybe use <input type='text' /> or simply remove the spinner arrows to improve the UI.
- Upon Reset, the custom input is not being cleaned up.
- Look out for the NaN values which are coming up whenever the custom input gets cleared up and then focused again.
- @BrazilianMemeMaster@Beats-Ayush
Definitely agree that the Javascript was the most difficult part. Some points -
- Vanilla JS will definitely require a lot of functions.
- I used React as a means to counter that.
- Other than that the window is reloading each time you reset. Instead use state to do that for you.
- Instead of setting
<input type='number' />
, maybe use<input type='text' />
or simply remove the spinner arrows to improve the UI.
Marked as helpful - @jrmy-dev@Beats-Ayush
@jrmydix I like the JS here. The only feature which might cause some trouble is the responsiveness. For the
tip percentage buttons
instead of using flexbox you can use grid which will allow more control over the width and not collapse the buttons into a single column below 374px. Just my humble opinion.Marked as helpful - @Beats-Ayush@Beats-Ayush
I don't know how the white space is appearing to the right and the bottom. It looks alright on live site.
- @sparrowsl@Beats-Ayush
@benjithorpe The solution looks great. Some issues you might wanna have a look at:
-
Use the same icons as provided in the images folder of the starting project.
-
Use the
hover
state on the buttons as shown in the project.
Marked as helpful -
- @AlsoShantanuBorkar@Beats-Ayush
ShantanuBorkar Excellent solution. Some places where you can improve your code-
-
Use the
background-color
for the body. -
Use
background-color: transparent;
in hover-state to pick up the color underlying the button. This way you can avoid applying the individual colors again. -
Use exactly one
<h1>
and semantic element landmarks like<main>
for better code readability and avoid accessibility errors. -
Use
max-width
to restrict the width of the body(Edit: not body but the card component which holds all the contents) to increase responsiveness.
Marked as helpful -
- @znair96@Beats-Ayush
znair96 Excellent work. The solution looks very close to the design. Some places where you may want to improve your code
-
While decreasing the width of the screen, before reaching the mobile layout i.e. in tablet layout, the flex items are stacking up on each other which is due to the
flex-wrap
. This is not a very good user experience in my opinion. You can usemax-width
instead to restrict the width. -
background-color
is missing. -
You have forgotten to add the hover state on the buttons.
-
- @Li-Bee@Beats-Ayush
@Li-Bee Excellent work completing the challenge. A couple of places where you might want to improve your code
- Put the screen-reader class outside of the main section so that it does not interfere with your content.
- To increase responsiveness, use max-width to restrict the width of the card container.
- border-radius simply gives a rounded edge to the border. It doesn't affect the div container.
Marked as helpful - @Samadeen@Beats-Ayush
@Samadeen Great solution. A couple of places where the code might get better
- Add a max-width to your container in mobile design so that it does not take up the entire screen.
- Remove min-width:50% from your image class and instead go for width:50% so that the image class and the content class get even space in the card.
- Finally don't use pixels in height, border-radius. Instead use rems, ems.
- @khshakib@Beats-Ayush
Hi Kamrul Hassan,
Other than the Accessibility and HTML issues, a couple of things will get the look closer to the solution-
- A border-radius to the card.
- Appropriate font-family and font-weights.
- max-width to restrict the width of the card when you are going to mobile design.