Latest solutions
- Submitted about 3 years ago
Yet another todo app
#emotion#react#react-testing-library#typescript- HTML
- CSS
- JS
- Submitted over 4 years ago
Job listings with React, Sass modules & snapshot testing with Jest
- HTML
- CSS
- JS
Latest comments
- @RikvanderSarSubmitted over 3 years ago@alex-kim-devPosted over 3 years ago
About that overflow / hamburger menu issue, right now you're applying
overflow-x: hidden;
to every element using universal selector. This is kind of radical approach, some elements might still require the default overflow to be displayed properly. I think we should be more precise with that. The navigation menu hasposition: absolute
, and the closest positioned ancestor element is.container
- so the menu is positioned relative to the.container
. Applyingoverflow-x: hidden
to this element will fix both issues. (And don't forget to remove overflow from everything).Great job on this challenge 🎉
Marked as helpful1 - @dannzdevSubmitted over 3 years ago@alex-kim-devPosted over 3 years ago
Hi Daniel, good job on this challenge! I've been looking through it and here's what I want to mention:
- it's not necessary to wrap every image in a
<figure>
element, it's required when you need to add a caption to it:
<figure> <img src="elephant.jpg" alt="Elephant at sunset"> <figcaption>An elephant at sunset</figcaption> </figure>
- the
alt
attribute with description must be present on images, it helps screen reader users and displays on the screen if an image isn't loaded - the margins of body and main elements collapsing, resulting in weird body positioning (with an offset on the top). There's a great post on the topic
- the
<main>
element already has semantics,role="main"
is not necessary - use either
<a>
or<button>
, but don't mix both like<a role="button">
Good luck!
Marked as helpful1 - it's not necessary to wrap every image in a
- @tlbodrickSubmitted over 3 years ago@alex-kim-devPosted over 3 years ago
Hey Tamera, the cards resize vertically because the height of their content changes, as you resize the window. It's possible to make them of the same height if we apply
align-items: stretch
to their flex container, but then we should make some other changes:/* 750px+ */ .main-component { margin: auto; display: flex; flex-direction: row; /* row is the default value, can remove this */ justify-content: center; /* doesn't do anything in this case */ align-items: stretch; /* change center to stretch, it's the default, so we can remove it too */ max-width: 920px; } .main-component { margin: 0 1.563em; height: 100vh; /* to remove, we want the height of our cards to be auto */ }
The cards are ready, now we need to center the whole component on the screen
body { height: 100vh; /* make body full screen */ display: flex; /* now the .main-component auto centers, because it has margin: auto */ margin: 0; /* remove the default margin so we don't get vertical scroll */ }
That's it, good luck & have fun on the next challenge!
Marked as helpful0 - P@tedikoSubmitted over 3 years ago@alex-kim-devPosted over 3 years ago
Awesome job, Tediko! I see you put a lot of effort into this project. I like that everything is well organized, files, code, commits, it's really easy to navigate through and find out how things are connected to each other. And great job on writing readme! Here is some feedback:
- animation feels slow, I would adjust duration to be around 200-300 ms
- accessing local storage might throw errors in some cases, for example if a user turned off all cookies in Chrome, some older browsers might throw even if only third-party cookies are blocked. So It's a good practice to wrap it in a try-catch block
- there are a few places with a lot of nested if-else statements, like in
formValidation.js
. That's increasing cyclomatic complexity and makes code hard to understand, especially for people not familiar with the codebase. Try to avoid nesting conditions. Ideally it should look like this:
() => { if (conditionA) return value1; // the following code will be executed only if(!conditionA) if (conditionB) return value2; // the following code will be executed only if(!conditionA && !conditionB) return value4; }
- would be nice to work a bit on loading performance. I had DOMContentLoaded: 1.99s on fast connection and 5.32s on simulated "fast 3G".
Good luck and until the next awesome project!
Marked as helpful3 - @die-lowenkoniginSubmitted over 3 years ago@alex-kim-devPosted over 3 years ago
Hey! A little suggestion on the
border-radius
. Do you know you can apply it only to the container along withoverflow: hidden
so it cuts the pointy angles of container's children? You can use it for larger screens, and for mobile just set the radius to 0.Also you can use Codepen's full page view link to make a nicer screenshot, however it's still going to mess up the a11y/html report.
Nice job overall and have fun coding!
Marked as helpful1 - @Juveria-DalviSubmitted over 3 years ago@alex-kim-devPosted over 3 years ago
I'd suggest relying more on box model (margin, padding, borders, width) and normal flow than applying
position: absolute
(and thus creating more flows). Probably this is what's caused the issue. Good luck! 👋Marked as helpful1