Looking to hire developers?
@HYDROCODER
@JulianIfesiokwu
Submitted
Please see my result here. The mobile view starts from 320px and the desktop starts from 741px. I could have done for tablets but I realized that's extra work because the requirements are just for desktop and mobile, I could do it but no need. Please any other feedback is greatly welcome. I am really happy with the JS. All complete.
@HYDROCODER
Posted
Hey there! Great work! There are a few suggestions I would suggest if you don't mind:
You do not need two breakpoints; just follow a mobile-first approach and then you can use one breakpoint for larger screen sizes.
Below the 741px breakpoint, the whole component extends to the entire viewport and it doesn't look good; all you have to do is set a max-width on your component and it will not extend beyond that. You can change this max-width for larger screen sizes, as it extends even at larger screen sizes.
There is a js issue with you solution. Suppose the user enters all the inputs and get the total bill and tip amount, if he changes either the bill or the tip-percent inputs after this, the total bill and tip amount do not change, unless the user changes the number of people input. The problem is that you have used a click listener just for the number of people input and as a result, the output changes only if number of people changes. If I am correct and if you can just replace the blur or focus listener for your inputs to change listener, every time the user changes any of the inputs, the total bill and tip amount changes in real time. I would suggest you ponder around with this, but I guess its fine as the user can just reset the whole thing and it works again.
Again, your solution works; what I have suggested above is just another improvement which you can try if you wish.
Hope it helps :).
Marked as helpful
@quocbao19982009
Submitted
Hi, this is the first JavaScript project that I have done so any feedback regards the improvement of the code is appreciated. I am looking for tips to improve my code quality, I feel like my code is very messy and hard to understand for an outsider. So what tips, principles should I follow so it can be cleaner, more understandable, and more effective? Thank you!
@HYDROCODER
Posted
Hey there! Good attempt on this challenge! There are a few suggestions I would suggest if you don't mind:
Even if the number of people is set to zero, the bill and tip are calculated for a single person without showing an error that number of people has not been mentioned. This needs to be corrected in app.js.
You have used a closing input tag for custom tip percent, but it is not necessary. If you are wondering how input tags get the labels, there is a specific tag for it called label tag that can be used for each input. In fact, you can use label for the bill input and the number of people input as well, instead of placing the headings in h2 tags. To place things inside the inputs you have used the placeholder but for placing things outside the input, and associate with it, you can use the label tag with the for attribute.
You can put comments in your code to make it easier for yourself or for anyone viewing your code. You don't have to write comments for every line of code. You can just write about the element to which a group of styles belong to, like a header or a footer, or any element.
You may learn more about semantic HTML in mdn or any other resource. This will improve your HTML markup.
You can use form elements for all of your inputs, and radio buttons for the tip percent instead of simple buttons. Styling form elements is another headache, but I guess its worth it for better accesbility.
Hope this helps :).
@jerry-the-kid
Submitted
Any feedback is appreciated. Thanks
@HYDROCODER
Posted
Hey there! The solution works and looks good! There are a few suggestions I would suggest if you don't mind:
When user clicks on the input fields of number of people or bill amount, a border appears and it sort of shifts everything and alters the placement slightly. You can use a box-shadow instead to achieve that effect.(inset type box-shadow)
When reset is clicked, the tip percent previously selected is not unchecked again, so I would suggest to remove the class "splitter__tip--active" from the button by adding a few more line of js in your reset functionality. Also, you can disable the reset button once it is clicked.
Hope this helps :).
Marked as helpful
@keeks05
Submitted
I had a question in relation to how I positioned the elements in my card; I relied on using the margin property, and I did not feel like that was the best way to go about positioning the elements.
@HYDROCODER
Posted
Hey there! Good attempt on this challenge. Regarding the positioning, there are a few things I would suggest if you don't mind:
Use more semantic elements in your HTML markup. For example, don't use divs everywhere. Wrap the whole thing in a main tag instead, use section or article tags if you wish but for this challenge I guess it fine with the divs, and for the stats, use ul+li semantics instead of plain divs. That way, you can position them the way you want conveniently and it is more accessible.
You have used fixed height and width for the card. Although the card is small and using such fixed sizes is fine here, I would suggest to not use them. This can cause horizontal scroll issues and can even break multiple elements of your page. You may use percent widths (like 80%, you may toy around it) and a max-width in px (like 800px; you may toy around it), to prevent it from stretching to the entire view port and at the same time have some gap at smaller screen sizes.
You have used margin-top and padding to give gaps between elements. It is commonly used to give gaps and you are fine there, but you can use better units instead of px like rem or em. Just remember this: padding should be used for giving spacing for the content inside the element, and margin is used to give spacing relative to other elements; this is just a general rule to follow and it can be broken at some places.
Hope it helps :).
Marked as helpful
@Daniellios
Submitted
Couldn't make buttons stay different color when pressing the other ones. In terms of calculation, you have to re-enter the percent value or re-click the percent button whenever you change the bill or number of people.
Any help on how to do it would be appreciated!
@HYDROCODER
Posted
Hey there! Your solution works and it looks good.
For your button problem, I would suggest that you use radio buttons instead; radio buttons have a :checked pseudo selector which can be used to make the buttons have a different color when pressed and stay with it unless you press some other tip percent. In short, it would be better to scrap the buttons in the "percentages" div and use <input type ="radio"> form inputs instead. Although styling them is another issue to take care of, using radio buttons is more semantic and gets the job done.
You can put some background color (grey) on your input fields (number of people, custom and bill).
You can disable your reset button once you click it and use it as default state for that button, using disabled property in your js code (document.querySelector('reset__button').disabled=true) and put a styling in your css for the disabled button using :disabled selector.
You have used a transition of .7s in your container. When zooming in or out of the viewport, it is taking it slow. You may remove it if you wish to since it has no purpose. In case you are using it for hover effects, just use the same transition in button: hover itself instead of applying it to all elements.
Hope this helps :).
Marked as helpful
@AthreyaG4
Submitted
I cant seem to remove the big gap in desktop view for the last card. Any suggestions appreciated.
@HYDROCODER
Posted
Hey there! Great job! You have even put media queries for the medium sizes and it works amazing! The gap may be from the fact that you used .9fr for the columns in the styles of your main tag. Just use grid-template-columns: repeat(4,1fr), give it a width and a max-width (for starters, try this: width: 70%; max-width:1200px and change it around if you wish). The gap will be reduced and all the cards as a whole will not stretch for larger screen sizes. You may even vertically center the whole thing for larger screen sizes using grid or flex on the body.
Hope it helps :).
Marked as helpful
@r-jkrishna
Submitted
All feedbacks are welcomed 😊
@HYDROCODER
Posted
Hey there! Great job on this one. I would suggest the following if you don't mind:
You can put a max-width on your content so that it doesn't stretch at larger screen sizes.
You have put the code for the svg of the logos in the HTML itself. You can just use an image tag to link to the svg from you HTML.
You don't have to put the width of 100vw in your body. Body tag by default has a width of 100%. Hope it helps :).
@azzykesuma
Submitted
any feedback will be appreciated!
@HYDROCODER
Posted
Hey there! Good job. There are some things I would suggest here if you don't mind:
Do put some comments in your code. It will be helpful to others as well as for yourself if you ever need to look at it again.
You have some accessibility issues. I would suggest to go through articles on semantic HTML if you wish to correct them.
You can remove the arrow buttons in your bill and no. of people input fields using the following code:
/* Chrome, Safari, Edge, Opera */ input::-webkit-outer-spin-button, input::-webkit-inner-spin-button { -webkit-appearance: none; margin: 0; } /* Firefox */ input[type="number"] { -moz-appearance: textfield; }
You have put a default tip percent of 15% and a default number of people. I would suggest you to remove that by making changes in your js code.
Hope it helps :).
@khazin
Submitted
Quite a fun challenge. feedback appreciated.
@HYDROCODER
Posted
Hey there! Good solution! There is just one issue I noticed with the testimonials. At around 670px, before your tablet breakpoint, the gap between the testimony--customer and testimony--quote becomes very high, and it can be from the fact that you used a justify-content: space-between for the testimony--wrapper class. Try centering the whole thing, or just reduce the gap (by adjusting the padding or the min-height you set for the wrapper). Otherwise, you have done a good job by using mobile-first workflow and putting some comments in your code. Although it works, try not to set fixed widths; do try using percent widths instead.
Hope it helps :).
@AsleyR
Submitted
I learned a lot about how to make responsive designs.
Does it matters that much that if it is not scale up to big screens? (think 4k screens and alike)
Are the responsive units I used properly used?
Thanks in advance for any feedback :)
@HYDROCODER
Posted
Hey there! Good work on this one. There are a few things I would suggest if you don't mind:
You have defined many media queries (3 to be precise). As a result, the cards are overflowing at one breakpoint (around 540px) and at some others it is sort of getting crushed, and since you have used overflow: hidden, it is disappearing to the right. Try taking up a mobile first approach. In this challenge, there are three cards and they all cannot fit in one row unless its a wider screen like a laptop. After completing the design for mobile, you can set the media query for larger screen sizes, and I guess just one media query would be enough, at around 1000px. If you wish to, you can define one for a tablet as well and put two cards in one row and center the third in another row using css grid, but I guess for this challenge just one is sufficient.
"Does it matters that much that if it is not scale up to big screens? (think 4k screens and alike)". While such screen sizes are rare, you can still manage to just put in a max-width on your main-wrapper and it will not stretch beyond it. It will just appear zoomed out a bit.
You centered your main-wrapper using equal margins to the left and right. Use a percent width instead like 90% or 80% and set the margin to 0 auto. It will be centered horizontally and you don't have to worry about what values to put in the margin anymore, as you can toggle around with just the percent width.
"Are the responsive units I used properly used?": Try using em for padding. It will scale with the font-size of your element if you ever set it to something different, and it will still look have the same spacing around it. For margins, you can use ems too, but I usually prefer using rems for margins as I don't want spacing to scale up or down with font-size, and I want to be more conscious about the spacing between elements. For font-sizes, you may use rems but never use em, it can get real messy with em.
Hope it helps :).
Marked as helpful
@GK-Production
Submitted
This is the fourth challenge I've done here and I would be happy if you leave some feedback on my code or solution. Thank you.
@HYDROCODER
Posted
Hey there! Great solution!
There is one thing I would suggest, although I must say I am nitpicking here. Just put some comments in your css; it can be boring writing it up but it will be helpful in the long run in case you need to see the code again as well as for anyone else viewing your code.
Also, your breakpoint seems a little low although it may be from the fact that you have used a desktop-first approach. The stars get congested in the range of 800px to 1000px. Although its your cue to take up the approach you find comfortable with, I would suggest a mobile-first approach; you can pick the breakpoint easily and don't have to worry about layouts at first but just about the colors or vertical spacing in mobile view and once that's done, can easily scale up the viewport and find the breakpoint you find best for larger screen sizes.
Hope it helps; again great job! :).
Marked as helpful
@chillcodemao
Submitted
UPDATED code:
@HYDROCODER
Posted
Hey there! Your HTML markup is very semantic so there are no accessibility issues at all! Good work!
You did use flex box a lot. I would suggest to use a class with the common flex styles and just use this class wherever you wish to use it. Layouts can be difficult so do be patient with it. Try flexbox on different challenges involving layouts and you will get the hang of it. Use margins to position things instead of padding. Padding work better when they are used to give spacing inside an element, not outside. Margins work better when they are used to position an element relative to other elements.
Never use fixed heights and widths. For this profile card its fine since it is a small element but larger elements take a toll when you do that and it can be a real headache to fix it when we set a fixed height or width.
Hope it helps :).
Marked as helpful
@S-A-M-0
Submitted
Your suggestions mean's a lot
@HYDROCODER
Posted
Hey there! Good work. I would suggest the following if you don't mind:
Put some comments in your css. It will be helpful to you as well as for anyone viewing your code.
Do use ids, but rarely. ID's are very specific selectors and they can become messy when you have lot of elements in your HTML markup. For this challenge, I guess its fine but do be cautious about it. Use classes instead.
Never set fixed heights and widths for anything unless something very specific.
For the stats at the bottom, although it works, to be more semantic and meaningful, use ul and li's. Do learn more about semantic HTML to solve the accessibility issue.
Hope it helps :).
@Sloth247
Submitted
Any suggestions to improve are welcome.
@HYDROCODER
Posted
Hey there! Great work on this one.
Regarding the height, you may just put min-height:100vh in the body styles. Never use fixed heights unless for something specific and this is not the one.
When wondering about breakpoints, just ask yourself this question while increasing the width of your viewport from mobile screens, using Dev Tools: at which width or more can my design look better (Talking from a mobile-first perspective)? Once you get it toy around with it and it should work fine.
Hope this helps :).
Marked as helpful
@soransh-singh
Submitted
Fun Challenge ... Feedback on code or design will be really appreciated and If you can give feedback on my js code that will be really helpful...
@HYDROCODER
Posted
Hey there! It looks and works great!. While I observed nothing wrong, I would suggest the following if you don't mind:
Do put comments in your code. It will be helpful for anyone using or seeing your code as well as for yourself if at all you need to correct it again.
There is an accessibility issue regarding inputs. You have used input elements without labels for them so screen readers won't be able to identify them. I would suggest to use labels instead of "para" divs for the headings and place it above the inputs. This way it will be more semantic, more accessible and less code in html.
You have used buttons for tip percent input. While it does work out great and styling it is easier, I would suggest to use radio buttons although styling them is another headache to take care of. That way once the user clicks a tip percent, the background color stays as it is using :checked pseudo-class selector, as well as it would be more semantic and accessible.
Hope it helps :).
Marked as helpful
@nguyenvn7
Submitted
Thank you so much for your feedback :)
@HYDROCODER
Posted
Hey there! Good attempt on this one. There are areas that you can improve so I would suggest the following if you don't mind :
Do put comments in your code, as it will be easy for others to help you, as well as for yourself, if you need to refer any part of your code.
Do use classes instead of ids. ID's are very specific selectors and they will mess up or increase the amount of code needed to write when compared with classes.
Try mobile first approach instead of desktop first approach. It will make writing CSS code a whole lot easier.
For correcting your accessibility issues, I would suggest you to look up on semantic html here. Instead of wrapping everything in a div, try using main or anything more semantic.
If two or more elements in your page use the same styles, use a class in your CSS code and use it in both the elements in html.
Never set a height on anything unless necessary for some very specific purposes.
Structure your html as per the desktop-version, style first as per the mobile version and then for the desktop version.
Hope this helps.
Marked as helpful
@Evanny
Submitted
Hello, any comment about my code is welcome, thank you
@HYDROCODER
Posted
Hey there! Good job on this one. It is responsive and good animation for the attribution!... I would like to suggest the following if you don't mind:
@Navin964
Submitted
My First try, Kindly give your feedback.
@HYDROCODER
Posted
Hey there! You have done a good job on this challenge. Although I am not an expert, I can give you a few suggestions if you don't mind:
Try using a mobile-first approach; trust me when I say this because I too used to do it first-desktop-then-mobile, and it was not a good experience every time. Once you are done doing it mobile-first, work your way up to desktop, with appropriate media queries at certain breakpoints, where you think the section can look better, if the cards are placed a little differently. You can use one media query for the desktop itself and if you find that the tablet version can look a little different you may use a media query here too. For this one I guess just one for the desktop is good enough.
Centering elements both horizontally and vertically can be quite a drag but doing it horizontally is quite a breeze with margin: 0 auto. I observed that you used margin: 0 500px 2.5rem; to center your description para. Use margin: 0 auto instead of it and give it a certain width you see fit, like width: 50%. To prevent this percent from making your para get into one single line at very large widths of the viewport, set a max-width property to a container that wraps around the whole code, like a div or any semantically correct html element. There are many other ways to do this but this is the one I feel is easier to do.
You can use css variables to use colors at various places instead of hard-coding it to the properties, although it is up to you to decide, as naming those variables can be confusing sometimes.
4.You can group all the common styles for those cards under a single name like card, use this name as a class for all the 4 cards, and then use extra class names beside it or ids like card-1,2,etc..for applying styles unique to those cards itself. Some may say ids should be rarely used but for this challenge I guess its fine.
Marked as helpful
@HYDROCODER
Submitted
Used css grid to solve this one and realized it made things easier instead of making it complex once we get the hang of it; Used ul+li semantics instead of using just divs everywhere; I used a box-shadow different from the one given as making one always seems hard and weird in pondering with the spread, blur, using multiple shadows... I tried centering the whole thing vertically too...do give your feedback if you find something wrong with the code or the site itself.
@HYDROCODER
Posted
I saw those 5 HTML issues and 4 of them are regarding missing the alt attributes of 4 images. I assumed them to be decorative so I just skipped the alt attribute and also used the aria-hidden to prevent screen readers from reading them in apple voiceover; turns out I must not skip the alt but include it with empty tags. Regarding the section with no heading: I used font weights instead of headings for those four words; this needs to change as it can prevent screen readers from understanding what the context is when I have clearly wrapped it with section tag.... so change the tag or use headings, either way.... learn more about such accessibility errors and never skip the alt attribute in images unless for very rare cases..
