@CarlosEduJs
Julianna Messineo
@mathematiCodeAll comments
- @mathematiCode
I really loved how you implemented the FiltersContext with the addFilter and removeFromList functions included. I just learned context so I didn't think of grouping those functions together into a custom hook. I noticed that you used span tags for the filters but this would make it hard for keyboard users to click them if the only way to do it is with a mouse. Anything that needs to be clicked should always be a button, and then you can just remove the default styles. Unless it's a link to an external page, then you can just use an anchor tag.
Marked as helpful - P@04stefke@mathematiCode
Nice Job! I had never heard of react-use-cart so that was interesting to see how you used that! It seems that your Confirm Order button isn't working. I don't see any onClick on that button in your code. I used Headless UI to make my modal.
Also make sure if you have an image that you're using as a button that you wrap it in a button tag instead of a div so it's keyboard accessible for users that are unable to use a mouse. Like this but you'd also have to get rid of the default background color that comes with buttons.
<button className="border rounded-full p-2 cursor-pointer" onClick={() => updateItemQuantity(product.id, isInCart.quantity - 1)}> <img src="./assets/images/icon-decrement-quantity.svg"/> </button> - @faaazy@mathematiCode
This is a pretty challenging project to make with just javascript. But you did a great job!
One note: For anything that the user needs to click as a button, make sure you wrap it in a button tag so it is accessible to people who only use keyboards and are unable to use a mouse.
<button class="decrement-button"> <img class="icon-decrement" src="assets/images/icon-decrement-quantity.svg" alt="icon-decrement-quantity"> </button> Then you can add the event listener to the button instead of the image so it can be clicked without using the mouse at all.
Nice job with the modal, that can be tricky. Sometimes people will want to close it just by clicking out of it in the backdrop area. But that is a lot easier with React; you can just use a component that someone else made.
All of your CSS looks great!
- @daniyal-abbassiWhat are you most proud of, and what would you do differently next time?
I do not geniually write this code.it was too hard. but I re-write it and examined it and now it's... easy:) i would write it with react next time since i want to learn react.
What challenges did you encounter, and how did you overcome them?write a function to change the order count and change the total count and do calculations all at the same time was so overwhelming and hard. i did everything but I couldn't find a solution so I began to search to see if anybody did it. fortunately there were too many people.
What specific areas of your project would you like help with?that "at the same time" functions which calculate.
@mathematiCodeThis is a pretty challenging problem to solve with just javascript. But you did a great job! I'm learning React too and the state management tools React provides definitely helps with the multiple different elements updating at the same time.
One note: For anything that the user needs to click as a button, make sure you wrap it in a button tag so it is accessible to people who only use keyboards and are unable to use a mouse.
<button class="decrement-button"> <img class="icon-decrement" src="assets/images/icon-decrement-quantity.svg" alt="icon-decrement-quantity"> </button>Then you can add the event listener to the button instead of the image so it can be clicked without using the mouse at all.
Nice job with the modal, that can be tricky. Sometimes people will want to close it just by clicking out of it in the backdrop area. But that is a lot easier with React; you can just use a component that someone else made.
All of your CSS looks great!
Marked as helpful - @ShoaibShujaWhat are you most proud of, and what would you do differently next time?
I designed the layout easily by using CSS Flexbox and Grid for the trackers. The script was also easy, but I had to write a lot of code but honestly, I think I could have done a bit better. 😔
What challenges did you encounter, and how did you overcome them?I usually use a lot of flexboxes due to its flexibility in different occasions specially when using different media queries but this time I decided not to use it and honestly, I was not satisfied with the results because I had to write a lot of code while using media queries and designing the page to smaller devices.
What specific areas of your project would you like help with?I would really appreciate if someone could review my JavaScript code and give me some suggestions or feedback. Thanks. 😊
@mathematiCodeReally great job! Your javascript actually helped me figure out my solution to this because I was confused about fetch and using json files and had to discuss it with my tutor. Your code was very easy to read and understand!
One thing that may be helpful to make responsiveness easier using grid is
grid-template-columns: repeat(auto-fit, minmax(180px, 1fr));This tells the browser to fit as many boxes as possible but make them at least 180px each and put the rest on the next row and so on. It is so useful for making grid-like designs responsive, if you don't mind sometimes having an odd amount of items on the last row.
Marked as helpful - @npc-makinika@mathematiCode
Your layout looks great! I know color matching is annoying but luckily frontendmentor gives you the exact colors in hsl form in the styleguide that you download with the project. I didn't realize this at first so I thought it might help you.
- @kaustubhyaWhat are you most proud of, and what would you do differently next time?
Tried making it responsive. For the first time implemented flexbox properly.
What challenges did you encounter, and how did you overcome them?Alignment. Difficulty with responsiveness, positioning using flexbox
What specific areas of your project would you like help with?Comments on how to improve my code writing and feedbacks for this project and my future projects.
@mathematiCodeHello Kaustubhya, your project looks very close to the design, nice job!
I noticed you have multiple h1 tags which can be considered an accessibility issue. https://www.a11yproject.com/posts/how-to-accessible-heading-structure/#one-h1
I did this project using grid and subgrid, I like how you did it using flexbox and it seems like it was smooth that way too!
Another thing I did differently is I did my button:hover all in one by changing the background color to be transparent. I had to use !important to get it to override the rule for the id though.
I saw that you used 15 pixels for the paragraph font size. I just learned from Kevin Powell on youtube that you shouldn't use pixels for font sizes because if someone has their browser set to make text larger due to vision difficulties, it won't change if it's set in pixels.
Overall, great job! The layout looks great at all screen sizes and the cards are centered nicely!
- P@medic-codeWhat are you most proud of, and what would you do differently next time?
Getting the components in a responsive manner. I would start with CSS grid for desktop view first.
What challenges did you encounter, and how did you overcome them?Not too bad actually, mostly around centering the grid and getting the correct distances.
What specific areas of your project would you like help with?HTML Semantics CSS structure and layout approach
@mathematiCodeWhen I open your published site on chrome and firefox, the top title "Reliable efficient delivery" gets cut off. It looks almost exact to the design screenshot so I'm not sure why they don't match. I used grid in a similar way to you. I ended up creating a separate div to place the icons in the bottom right.
- @Jsnowoliv1420@mathematiCode
I was having the same problem where my card wouldn't center vertically. I had learned somewhere that "align items" was always for vertical centering but turns out, that's not true. Since we both used
flex-direction: column, "align items" would align the items to make a straight vertical column. So since our flex direction is column, we would have to usejustify-content: center;in order to center the column along the vertical axis.Marked as helpful - @amna1526What are you most proud of, and what would you do differently next time?
I am proud of being able to finish it...
What challenges did you encounter, and how did you overcome them?The challenge was to do everything on my own mainly but when all was said and done, I really felt proud of myself.
What specific areas of your project would you like help with?The width property mainly, idk which unit to use for it
@mathematiCodeIt looks good! You could include a top margin or center the card within the body to center it more.
- @polinagusakovaWhat are you most proud of, and what would you do differently next time?
.
What challenges did you encounter, and how did you overcome them?.
What specific areas of your project would you like help with?Responsible design
@mathematiCodeI can't leave any feedback on your code because I can't see it (maybe it's not public?). I really enjoyed watching [Kevin Powell's video] (https://youtu.be/B2WL6KkqhLQ?si=qH2z5o9TlgzcRfed) on the project after I was done. Maybe this would be helpful to you too.
- P@JoeWebDevelopmentWhat are you most proud of, and what would you do differently next time?
Use more utility classes, and rely more on global variables. I'm proud that this one has a lot of responsiveness.
What challenges did you encounter, and how did you overcome them?I had a challenge with the picture/image changing, but googled the solution using a srcset and media to match the media queries. I was also having issues of white space as it grew, but just used a break point of the media query to prevent that and max the width along with creating a mid-width to have consistency.
What specific areas of your project would you like help with?Please give it a look over. I know I need to add more for accessibility and screen reader users but that is not an area I have much knowledge in.
@mathematiCodeI enjoyed how you used CSS variables for the fonts and colors. I didn't think to do that. I also CSS grid and I like how you defined the grid template under the media query. My solution used "display:none" to hide each image depending on the size but I can see how that might not be ideal for download times. I'll have to look into srcset.
This was pretty easy for me to understand, and I learned from seeing how you did it, so thanks!
Marked as helpful
