@RikvanderSar
Submitted
No specific questions about this one. All feedback is appreciated!
Looking to hire developers?
@alex-kim-dev
@RikvanderSar
Submitted
No specific questions about this one. All feedback is appreciated!
@alex-kim-dev
Posted
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 has position: absolute
, and the closest positioned ancestor element is .container
- so the menu is positioned relative to the .container
. Applying overflow-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 helpful
@dannzdev
Submitted
I would like any feedback if its possible good or bad. I'm good taking feedback to grow. I just wanted to learn and be better. Hope you like it. Please feedback!
@alex-kim-dev
Posted
Hi Daniel, good job on this challenge! I've been looking through it and here's what I want to mention:
<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>
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<main>
element already has semantics, role="main"
is not necessary<a>
or <button>
, but don't mix both like <a role="button">
Good luck!
Marked as helpful
@tlbodrick
Submitted
For some reason, the cards resize randomly when changing the window size and I can't figure out why.
@alex-kim-dev
Posted
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 helpful
Hello👋!
This is an invoicing application build with ReactJS and styled-components. The application is used to manage invoices and allows the user to create, read, update, filter by status and delete invoices. There is an option in the app to switch between a dark and a light theme. All transitions are smoothly displayed by using Framer Motion library to create animations. It was by far the largest and most comprehensive project I have done so far. It showed me how important it is to plan so that you don't have to change things that previously worked well in the middle of the project. A valuable lesson!
useReducer
hook to manage application state. I noticed that my state logic getting more complex as the few elements of my state relies on the value of another element of my state. Read MoreuseContext
hook turned out to be handy. It allowed me to create common data that can be accessed throughout the component hierarchy without passing the props down manually to each level which in turn allowed me to avoid Prop drilling (also called "threading") that is the process you have to go through to get data to parts of the React Component tree. Read More(1). Read More(2)<ThemeProvider>
wrapper component. By leveraging the theme provider I only need to write my styles in one single object and invoke them in any component that is a descendant of that provider. Read More<input>
, <textarea>
, and <select>
typically maintain their own state and update it based on user input. In React, mutable state is typically kept in the state property of components, and only updated with setState()
. Then the React component that renders a form also controls what happens in that form on subsequent user input. Read MoreADA compliant
(which means the website should be entirely accessible using just keyboard) I prevent focus go outside the modal once the modal is opened. In this case, the focus trap turns on when the form or modal with invoice deletion/status change is opened. In order to create an accessible modal I followed this great tutorial that follow the WAI-ARIA Practices.useReducedMotion
hook. Based on whether useReducedMotion
returns true
or not we're passing different values to animate
. That replace position transitions with opacity. Read More(1). Read More(2)404 Page Not Found
error to the user. Try to enter a page that doesn't exist - like 'invoice-tediko.netlify.app/gotcha'You can find more about the things I used in the project in the README on github. I just wanted to point out most important things here.
Questions:
Bugs:
AnimatePresence
that allows components to animate out when they're removed from the React tree. I think build in isPresent state when clicking twice quickly didn't change the state. I overcame this problem with simple event.preventDefault()
with a setTimeout
function but..Additional feedback or a criticism will be appreciated Thanks! 😁!
@alex-kim-dev
Posted
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:
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;
}
Good luck and until the next awesome project!
Marked as helpful
@die-lowenkonigin
Submitted
Hi all, please checkout my submitted solution. It will be a pleasure to know your perspective, experiences, etc. when developing a website.
Feel free to comment on:
Challenges encountered:
Border Radius. I used a hacky way to do it. Please see the live site > scss > media queries > border-radius.
Sticky Footer. I used a hacky way to do this. (please see live site > html)
Notes:
@alex-kim-dev
Posted
Hey! A little suggestion on the border-radius
. Do you know you can apply it only to the container along with overflow: 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 helpful
@JuveriaD
Submitted
Not perfect but still trying to improve , I would like to know if I made any mistake in my code feel free to give your suggestion's. Thank You..
@alex-kim-dev
Posted
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 helpful
@emestabillo
Submitted
The side-by-side layout for medium devices looked awkward for me so I opted to stick with the stacked design. What's everyone's take on the tablet layout for this project?
For devices with <667px height --> I couldn't get it to work unless I use absolute positioning. Does it make sense to still cover these devices? (Please be nice) Thanks for the feedback!
@alex-kim-dev
Posted
I do basically the same as you did - leave the 1 column layout and give it max-width
to prevent content (especially text) getting too wide + centering
@emestabillo
Submitted
I'm thinking this solution could be more elegant, maybe using themes in css. I'd appreciate any feedback thank you :-)
@alex-kim-dev
Posted
Yeah, the css variables are very helpful for this kind of purpose. The alternative set of color variables could be used with a selector that targets the checked
state of the switch, eliminating the need to use JS for theme changing.
@zuolizhu
Submitted
Feedbacks are super welcome!
Almost pixel perfect 😆.
@alex-kim-dev
Posted
Hi Connor, I've been inspecting some of your solutions and I think you're doing a great job! I like the visual accuracy and how the code is organized. There's one thing that I spotted - some BEM elements which are nested inside other elements have this type of naming: block__elem1__elem2__elem3
. However it's not recommended in the BEM documentation (Guidelines for using elements - nesting). So instead of a markup like this:
// pug
nav.desk-nav
ul.desk-nav__menu
li.desk-nav__menu__item
.desk-nav__menu__item__underline
it's recommended to do it this way:
// pug
nav.desk-nav
ul.desk-nav__menu
li.desk-nav__item
.desk-nav__underline
Also I've noticed you have a lot of solutions using Svelte, so when I'll be trying it out I'll definitely come back to them and learn. Have a good one!
@mubaraqwahab
Submitted
Feedbacks are welcome, especially on the accessibility. Thanks.
@alex-kim-dev
Posted
Hey Mubaraq, great solution! Here are some of my thoughts:
aria-label
instead of adding a span with .visually-hidden
on the buttons, your solution works perfectly though, so that's just a preference;meter
element + wrap the paragraph above into a label
for that meter, just to be sure that assistive technology will announce this part.Good luck and have fun making the next one!
@emestabillo
Submitted
This was a true test in writing responsive code. Would appreciate your thoughts. Many thanks to my mentor @shahsilo for helping me get really close to the design!
@alex-kim-dev
Posted
Hey Emmilie! That's a really fantastic work! Your solution is pretty accurate compared to the design. Also I like how you structured your SASS - very convenient and simple. I honestly don't know what can be improved, but here is a little hint on using media queries in sass. I noticed you often write @media screen and (min-width: 992px) {}
. We can cut a little code here by defining mixins:
// mdUp means for medium screens (992px) and higher
@mixin mdUp {
@media screen and (min-width: 992px) {
@content;
}
}
then we can use it like so:
.class {
padding: 1rem;
@include mdUp {
padding: 1.5rem;
}
}
And that's it. Good luck and I'll see your next project!
@king-qais
Submitted
I might have a little issue making it responsive on mobile. Somehow there is less margin on the right side when viewing on mobile. Also whenever you open it mobile, the website appears slightly zoomed in.
@alex-kim-dev
Posted
Hello, the reason for this is that the cards have fixed width of 350px. If you open the page on 375px wide screen the total width (card + margin & padding) will exceed that limit, and the page will have a horizontal scroll. Typically the mobile layout is made using width: 100%;
to make sure it's always full width.
I'm having trouble leaving the responsive layout with Css. Can you help me ? Mainly with the responsiveness of the third box?
@alex-kim-dev
Posted
Hi Izabella, I see you are using css grid for the layout, currently the grid has 3 columns and 3 rows, but both the 3rd column and 3rd row are not occupied by anything. For this kind of layout a 2x2 grid would be enough.
Next up, the height of the boxes should be fluid - based on its inner content - so remove the height
from each of them and use auto
for grid rows. Css grid will make your bottom boxes of equal height automatically. That's it, nice job, keep up the good work!
Suggestions welcomed for improvement.☺
@alex-kim-dev
Posted
Great solution, Roshan! Pretty close to the design and responsive. There's a couple of points worth mentioning:
100vh
height and position the component inside using flexbox.<button>
elements instead of divs + add styling for states (:active
, :focus
, :hover
).Congratulations on solving your first challenge and good luck with the next one!
@adarshcodes
Submitted
@alex-kim-dev
Posted
Hey Adarsh, nice solution on this challenge! I like that you added some animation for cards, cool little effects on hover. And that fancy font that you used for the attribution is looking great. I looked into the code and i have some feedback for you:
height
set for every card can cause issues when the content inside changesGood luck!
@kushank1207
Submitted
@alex-kim-dev
Posted
Hey, your solution looks really well! I've just inspected it and there's some points worth mentioning:
max-width
and auto
margins on the sides could help with that@import
statement in the css file, wrapping the url address into url()
function should fix it @import MDN. Also it's a bit more benefitial in terms of performance to load fonts using <link>
Stackoverflow questionKeep up the good work!
@alex-kim-dev
Posted
Nice try on this challenge! Here are a few things you can improve:
:focus, :hover, :active
)aria-label
, eg 'Signup form' instead of just 'form'br
between the inputs@kushank1207
Submitted
@alex-kim-dev
Posted
Hey Kushank, nice work on this challenge! I have some feedback for you:
max-width
can help with that@developerbottle
Submitted
Tried using SMACSS architecture for my CSS. Definitely not perfect code. Comments on how to implement SMACSS better is highly appreciated!
@alex-kim-dev
Posted
I haven't tried SMACSS myself but your css looks very clean. As always, nice and pixel perfect solution!
@developerbottle
Submitted
Any feedback/like/bookmark is highly appreciated! 😉
@alex-kim-dev
Posted
I have two questions.
aria-hidden
attribute to the div
with a background image? I think screen readers won't annouce it anyway.I like your solutions, learned some good practices while inspecting them. Thanks!
@ksenius
Submitted
I tried hard on this challenge. It took me hours to configure Gulp for the first time. I also used CSS Grid for the first time (only in the footer, though) and made it work in IE 11. But the most challenging thing to me was to position background images.
Feedback is welcome and appreciated :)
@alex-kim-dev
Posted
Great work! The design, how the css is organized - it's just awesome!
I noticed i can't swipe the testimonials card again if the previous animation is not ended.
Looking forward to your next solutions.
@ksenius
Submitted
@alex-kim-dev
Posted
Pretty decent job! I like your gulp setup and the way you used BEM. Also the page matches the design very well! There're just 2 small things that i would add:
:focus
staterel="noreferrer"
attribute for the external links, here is more info