Emmanuel Lopez
@EmLopezDevAll comments
- P@jull20P@EmLopezDev
Looks great, good job on completing the challenge. A few points as far as the look and feel.
- There should be some more padding at the bottom of the page not having the last element right up against the edge of the window.
- Buttons should have a cursor of pointer to help give a better visual queue that it can be clicked on.
- The images are not sized well within the container, when the window is at certain sized the images get cut off.
- when the hamburger menu is pressed and the sidebar menu opens, if the window is resized to a point where there should be no sidebar menu you should have it run the close function instead of it remaining open and not looking great.
- Some challenge, after adding an product to the cart if something else is added have it add as a new item instead of replacing what is there. Also a better experience especially for mobile is when you scroll the header should stay sticky so the user can always navigate instead of having to scroll all the way back.
Marked as helpful - P@toshirokubotaWhat specific areas of your project would you like help with?
Any suggestions/comments/feedbacks are very much appreciated!
P@EmLopezDevLooks great. Good job on completing the challenge. I am able to open and close the side navbar and the resizing works.
Some feedback:
- There is a lot of spacing missing in between elements example the main image is missing some bottom spacing and so is the header.
- When resizing the window the images scale in a way that doesn't really look good. Example if you resize your window to 880 the main image looks small and adds too much spacing underneath itself causing a lot of whitespace.
- The overlay when the sidebar is open is missing. If you look at the mock ups when the sidebar is open the background behind it should be a darker color this is to help empathize the sidebar currently as it is, it gets lost with the rest of the content since it is all the same color.
- One thing to take into consideration is if the sidebar is open and the user resizes the window to a bigger size where the sidebar shouldn't show then the side bar should be closed and not remain open.
- Last thing and this is more for you to think about not really feedback. The header where the navigation is should be sticky meaning it stays on screen when you scroll. The reason why is because if scroll all the way down, the only way I can then navigate again is to scroll all the way back up. This slows the user down from getting to the next piece of content on the site.
- @NitiemaAllassaneP@EmLopezDev
Good job on completing the challenge, it looks good and I am able to submit a enquiry and also see any errors.
Some feedback:
- The
<aside>
tags aren't being used correctly. Aside tags are generally used to show content that is indirectly related to the main content and position a side from the main content. For your instances a<div>
should suffice. - Other than that I would say the look of the form is a little off from the mock ups. Some of the components are not properly spaced, sized, or even padded.
- Lastly as far as the error states go, a better user experience is once the user enters a valid input the error should go away and not wait until re-submitting. This will prevent the user from having to constantly re-submit to validate their entry. Also maybe add a height to the container where the errors show so that as the show and hide elements around it aren't moving.
Marked as helpful - The
- @limsaelWhat specific areas of your project would you like help with?
- The accordion itself. This was my accordion so any feedback on how to improve it would be welcome.
P@EmLopezDevGood job on completing the challenge, it looks great. Keep doing great work.
A few things to point out:
- The look of the card is a bit off there isn't enough padding on the edges of the card.
- When view on a smaller display, the card isn't centered on the viewport.
- When the display is made really big the background image doesn't take up the full width;
- Missing visuals for focus. If you tab through the page it should show you what is being focused.
- You can make the whole
accordion item
div have the onclick to open and close it makes for a better experience to be able to click anywhere and have it open and close especially for accessibility needs. - Also look into using the
<details>
tag for the accordion it is intended for these kind of components and has the ability to open/close built in so you don't even need JS.
- P@toshirokubotaWhat specific areas of your project would you like help with?
I appreciate any feedbacks/comments/suggestions. Thanks.
P@EmLopezDevGreat work, love your use of React and Typescript. Good job on completing the challenge and getting it to work.
Some feedback:
- The look doesn't quite match the design there are a few things off like the color specifically the card background color looks darker. The submit button isn't quite the right size and roundness. When a rating is selected a border appears around the ratings which shouldn't be there.
- P@leeport511What are you most proud of, and what would you do differently next time?
This project took me a lot of time, especially because I had holidays in the middle, but it felt great to finish it. My knowledge has improved significantly. I learned a lot through this project.
What challenges did you encounter, and how did you overcome them?I learned that I need to work on structuring my CSS better from the beginning. When I reached the part where I had to implement dark/light mode, I encountered many difficulties because I hadn't planned the project with that logic in mind from the start.
P@EmLopezDevGood job on completing the challenge, it looks good and works I was able to get through the test and get a score. I like how you created all the smaller helper functions it makes your code easier to debug and follow.
A few things that can improve:
- The background image isn't taking up the whole screen, you should add the image to the body tag so it takes up the whole screen even if the main tag has a min/max.
- There isn't enough padding at the table and desktop view, the submit button is right on the edge which isn't a good UI look.
- The error image isn't showing looks like it may be a wrong path.
- The submit answer button shouldn't have a hover effect, it should be disabled until an answer is selected. The hover state lighter style should be a disabled style and when a user selects an answer then it turns the darker color.
- When an answer is selected if anything outside the answers is clicked the the answer is then unselected visually which makes it look like there is no answer. Also once an answer is submitted and checked the user shouldn't be able to select anything.
- Although you light/dark themes works and is a good approach there is a simpler way of doing it by creating light and dark variables and just overriding them. You can check out my solution if you are interested.
- P@kephaloskWhat are you most proud of, and what would you do differently next time?
Using Single Responsibility Principle. At least for all UI-components (atoms and containers). In case of some custom hooks I'm not sure.
What challenges did you encounter, and how did you overcome them?Building a custom SliderBar. I never did that before. I programmed it step by step: first Layout (background,value,adjuster,container); than user interaction(Click,KeyDown,KeyUp,MouseDown,MouseMove,MouseUp); after that storage logic.
What specific areas of your project would you like help with?I want to know how to test an Error that is thrown in an async hook in jest.
I tried this and the error is thrown succesfully, but act() did not pass the error to expect(), so the test fails:
it("throws an error if copy to clipboard fails", async (): setup({ password: "testPassword" }); const element: HTMLElement = screen.getByTestId(testComponentDataTestId); expect( async () => await act(async () => { fireEvent.click(element); }), ).toThrow(new Error(ERROR_MESSAGE_PASSWORD_COPY_PREFIX)); });
As a workaround I changed that Error to a console.error, so I could spy on it and check if it was called. So this works, but I would prefer to throw an error at this point:
it("throws an error if copy to clipboard fails", async (): Promise<void> => { setup({ password: "testPassword" }); const element: HTMLElement = screen.getByTestId(testComponentDataTestId); await act(async (): Promise<void> => { fireEvent.click(element); }); expect(console.error).toHaveBeenCalledTimes(1); expect(console.error).toHaveBeenCalledWith( ERROR_MESSAGE_PASSWORD_COPY_PREFIX + errorMessage, );
Thanks for any help. :)
Password Generator using Best Practices in React/TypeScript/Jest/Redux
#jest#react#redux#sass/scss#typescriptP@EmLopezDevWow this is great, I love the approach with making every part of the app re-usable including the footer. Adding testing is also such a good touch, I am not too familiar with testing so I can't give you the testing feedback you are looking for. All I can say I am impressed with it all.
Some take aways from my perspective:
- Although the re-usability and abstraction is a great approach and maybe you were just practicing it or getting into the habit but I personally think this solution is overly abstracted and could have been a bit more simple. With the inception of files and components it does take a while to follow and put it all together during review. But again that is just my point of view, not saying what you did was wrong but a simpler approach for a simple challenge would make it easier to maintain, easier to test, and easier to debug. Just something to keep in mind.
- Aside from that the look of the app is a bit off from the design, some of the padding is off like around the button and body. Also the
color
of the text when the password is generated is incorrect.
Marked as helpful - @RomeoKsP@EmLopezDev
Looks great, something I over looked on mine that you added was the ability to reset anytime any value is enter which is a good user experience. Good job. It looks pretty much identical to the design.
Few things that can improve:
- If a custom tip is entered and then you click one of the tip buttons, the custom tip input should get cleared and not retain the entered amount.
- Another thing is you are using a lot of divs, there are other HTML tags you can use to make it more semantic for example wrapping the bill, tip, and people inputs in a
form
tag or using a few section tags.
- P@braismarquez2025What are you most proud of, and what would you do differently next time?
Me he sentido bien programando con javascript, no he tenido problema a la hora de seleccionar los elementos ni al crearlos.
What challenges did you encounter, and how did you overcome them?Me ha costado bastante iterar entre los contenedores "item" que he creado con js, ya que era necesario tanto para las imágenes como para el color del fondo del before. Otra cosa que me ha costado que quede bien es el grid en la resolución desktop, ya que los contenedores no ocupaban todo el espacio requerido, creándose márgenes gigantescos. Sobre todo el contenedor en el que cambias el tiempo (dia, semana o mes), ya que no se alineaba la altura con el grid, debido al before que contiene.
What specific areas of your project would you like help with?Cualquier mejora es aceptada ya que es el segundo proyecto que realizo introduciendo javascript.
P@EmLopezDevUltima cosa que se me olvido mencionar es que trate de usar etiquetas de HTML mas semantica no tanto
div
por ejemplo el div principal debe ser<main>
, cada tarjeta puede ser un<article>
y en ves de usa<p>
para daily, weekly, monthly usa mejor<button>
porque tecnicamente son botones. El p tag es para texto.Marked as helpful - P@braismarquez2025What are you most proud of, and what would you do differently next time?
Me he sentido bien programando con javascript, no he tenido problema a la hora de seleccionar los elementos ni al crearlos.
What challenges did you encounter, and how did you overcome them?Me ha costado bastante iterar entre los contenedores "item" que he creado con js, ya que era necesario tanto para las imágenes como para el color del fondo del before. Otra cosa que me ha costado que quede bien es el grid en la resolución desktop, ya que los contenedores no ocupaban todo el espacio requerido, creándose márgenes gigantescos. Sobre todo el contenedor en el que cambias el tiempo (dia, semana o mes), ya que no se alineaba la altura con el grid, debido al before que contiene.
What specific areas of your project would you like help with?Cualquier mejora es aceptada ya que es el segundo proyecto que realizo introduciendo javascript.
P@EmLopezDevSe ven muy bien, hiciste un gran trabajo. Te felicito.
Un pal de cosas que puedes hacer un poco mejor son:
- Parce que el gap es mucho cuando la pantalla se pone grande no se si te fijaste pero los cuadrito/cartas se separan demaciado.
- estado activo de daily, weekly, monthly no esta, cuando el usuario selecciona uno se debe mantener blanco para que sepan en que vista esta.
- El script.js esta bien por ahora porque no son muchas lineas de codigo, pero mejor seria que separara las cosas en funciones mas pequeñas haci se hace mejor leer y si en algun moment hay problemas es mas facil buscarle solucion si todo esta separado. Esto es un concepto en software engineer que se llama
separación de preocupaciones
https://www.geeksforgeeks.org/separation-of-concerns-soc/
Marked as helpful - P@imvan2What are you most proud of, and what would you do differently next time?
I'm most proud of my JS file and how much I understood what to do.
What challenges did you encounter, and how did you overcome them?Just getting the styles correctly. I did my usual way of working with CSS, then I tried to scale down.
What specific areas of your project would you like help with?I would like help with making my SCSS files better? Are there any responsive shortcuts that I could have done instead of using media queries?
P@EmLopezDevGreat job, looks very close to the design. The validation and submit work as well.
Somethings that could improve:
- The mobile view looks of, it doesn't take up the entire view height.
- Although you have validation on the input you should add the required attribute on it just incase anything isn't working in the JS or doesn't load properly it will prevent the user from submitting it without anything.
- A better user experience for the input is to remove the error state once the user begins typing again and not leave it in the error state.
Marked as helpful - @ostapyshynP@EmLopezDev
This is such a great solution. I love the approach and I am going to model it for the next challenge.
- Only feedback is the hero image gets really big as the window resizes.
- This also could have been done with just JS but I feel the less complexity the better but honestly that is me reaching for feedback because ultimately its a great solution.
- @nitinrs95P@EmLopezDev
Overall looks great, very close to the design and responsive.
-Only feedback I would say is try and get it a bit closer to the design. Some things or off like the hover effect colors, the version number on the purple button is the wrong color, and also the images in the middle should be a tile layout try using grid for that.
Marked as helpful - P@lenny131P@EmLopezDev
Looks great honestly, it responsive and looks pretty close to the design.
- Maybe center the main tag a bit more since it looks higher up than the design
- Also is better to use em instead of rem for media queries, they have a bit more advantage when it comes to accessibilty. This has a neat explanation rem vs em
Marked as helpful - P@lenny131What are you most proud of, and what would you do differently next time?
I used CSS grid for the feature cards.
What challenges did you encounter, and how did you overcome them?I had trouble getting the card colored border to work with the rounded corners of the card, using borders or box shadows. I ended up using a div with background color instead. I'd rather not having any style elements in the html code, but it works.
P@EmLopezDevHonestly this is a great solution. I love the use of
SCSS
mixins. The site is responsive and matches the design. Really good job.A few calls outs:
- Personally I would have made the section "title-wrapper" as a header above main. To me its looks like introductory content to the page. What do you think?
- Try using
clamp()
for font-size responsiveness as it gives a smoother transition when scaling the window. This is a neat tool to help calculate that Fluid Typography - Lastly its better to use
em
notrem
for media query as it has a slight advantage. This article has a section for media query Rems vs Ems
Marked as helpful - @Mnsa2024P@EmLopezDev
Overall looks great and is responsive, I like that you used SCSS with abstracts which is a great approach to writing CSS.
A few call outs are:
-
Although your style files can live within the assets and it works fine in such a small codebase, I would say it would be best to get in the habit of either a. created a sub directory in assets and house them there or b. which is what I have seen most people do is create standalone styles directory outside of assets and put them there. As the codebase grows it is much organized to keep all styling in its own directory to find them faster.
-
You can use less divs. Your main tag could be the card class since the card itself is the main content and instead of using divs you could use sections which would be a bit more semantic.
<main class="card"> <img class="card-img" /> <section class="card-content"></section> </main>
The less tags you use the easier it is to read and maintain.
- Lastly I see a mix of px and rem for font-sizes I would stick to rem mostly over px for better responsiveness and meeting accessibility needs.
Marked as helpful -
- @harshinichanduP@EmLopezDev
It looks good and responsive, I also like your usage of CSS variables very clean approach.
The design however is off, the fonts don't look the same, the image isn't loading could be an issue with the path, the padding wrapper padding looks off as well. I would say take some extra time to get it closer to the design.
- @glrodriperez98What are you most proud of, and what would you do differently next time?
I’m most proud of how clean and semantic the structure of the HTML is, and how the design is fully responsive with a minimal amount of code. The layout adapts nicely to different screen sizes, and the use of
flexbox
, custom properties, and semantic tags made the code very readable and maintainable. This was feedback provided to me on my last project and I finally grasped how to not solely rely on<div>
.If I were to do it again, I’d plan out the vertical spacing more carefully from the beginning. I spent some time tweaking margins and padding to get the vertical rhythm feeling just right. After some research, next time, I’d approach that aspect with a spacing system or utility classes to save time.
What challenges did you encounter, and how did you overcome them?One of the challenges I encountered was getting the vertical spacing of the card just right, making it feel centered without being too stretched, while also having a good amount of breathing room on mobile and desktop. It took a few iterations of adjusting
padding
,align-items
, andmargin values
to strike the right balance.Another challenge was making the layout both mobile-first and visually consistent across devices without overcomplicating the code. I overcame this by using
What specific areas of your project would you like help with?max-width
,flexbox
, and percentage-based padding as opposed to strictly pixels to maintain a scalable layout.An element where I struggled in this project, along with my other projects in matching the size of my solution to the solution provided by the design. When time to review my screenshot in comparison my solution is either too big or too small. If there are any tips or feedback that would help me gauge getting the initial size right at the beginning of the project that would be much appreciate.
Also any additional feedback around the lines of:
- Are there any accessibility improvements I could make to improve screen reader support or keyboard navigation
- Are there any tips or tricks with my solution to improve the experience without overwhelming the simplicity, along with making it more readable and scalable
Thanks!
P@EmLopezDevLooks great, your outcome looks pretty close to the design. Your HTML looks great, good choice of semantic tags.
Some feedback:
- From my experience your styles.css file shouldn't be inside the assets directory.
- The section tag that holds the p tag can be eliminated and just keep it as a p tag.
- The padding is off on the card class, it should be 40px.
- One suggestion is setting your HTML font-size to be 62.5% which is 10px, then you can better use rem. Example 16px would be 1.6rem, 24px would be 2.4rem and so on. I believe by default the font-size for an HTML document is 16px which makes it a bit more tedious to calculate what you want using rem. As you can see your .card element's padding is set to 1.5rem, which is 24px as opposed to the 40px it should be.
Overall it still looks real good. Good job.
Marked as helpful