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.
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 :).
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!
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 :).
Any feedback is appreciated. Thanks
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 :).
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.
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 :).
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!
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 :).
I cant seem to remove the big gap in desktop view for the last card. Any suggestions appreciated.
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 :).
All feedbacks are welcomed 😊
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 :).
any feedback will be appreciated!
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 :).
Quite a fun challenge. feedback appreciated.
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 :).
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 :)
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 :).
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.
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! :).
UPDATED code:
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 :).
