Skip to content
OverviewSolutions
21
Comments
23
Alex Kim
@alex-kim-dev

All comments

  • Alex Kim•1,325
    @alex-kim-devPosted about 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 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
    1
  • Alex Kim•1,325
    @alex-kim-devPosted about 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 helpful
    1
  • Alex Kim•1,325
    @alex-kim-devPosted about 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 helpful
    0
  • tediko•6,680
    @tedikoSubmitted about 3 years ago

    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!

    • The first time I used 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 More
    • Together with useReducer, the useContext 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)
    • In order to create a theme switcher and provide colors for components I used styled-components <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
    • When creating the form I learned what Controlled Component is. In HTML, form elements such as <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 More
    • To make application more ADA 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.
    • To animate the pages transitions and modals I used Framer Motion API. Framer Motion is an open source, production-ready library that's designed for creating creative animations. In order to support users who have enabled their device’s Reduced Motion setting and make accessible animations I used 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)
    • Handle 404 routes in React Router and provide a fallback component for displaying an imfamous 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:

    1. As I am changing the theme and many colors are changing, I wasn't sure how to make the color system. I end up with object for dark/light theme with colors of each element and a few colors that are common. How would you approach that?
    2. I didn't want to create a special component for my headings since they have no logic. I created helper utility called typographyStyles where I put all styles that I use across application. Not sure about that tho.
    3. I am curious what method you use to name your components. I personally stick to option #2 because it has the most pros for me and a little longer import doesn't bother me.

    Bugs:

    1. Clicking twice quickly on header logo (navigate to home page, which render Invoices component) causes component not to render. I am convinced that this is related to Framer Motion and 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! 😁!

    Invoice App - ReactJS, Styled-Components, Framer Motion, Webpack

    3
    Alex Kim•1,325
    @alex-kim-devPosted about 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 helpful
    3
  • Die Löwenkönigin•15
    @die-lowenkoniginSubmitted about 3 years ago

    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:

    • suggestion to better solutions (or avoid using 'hacks' :D)
    • some nit-picks to a more beautiful and responsive front-end design
    • development cycle used for faster and efficient process of building a website.
    • other advice related to this challenge

    Challenges encountered:

    1. Border Radius. I used a hacky way to do it. Please see the live site > scss > media queries > border-radius.

    2. Sticky Footer. I used a hacky way to do this. (please see live site > html)

    • from html, I added class of h-100.
    • from body, class="d-flex flex-column h-100"
    • from main, I class="flex-shrink-0"
    • from footer, I used container class inside the tag

    Notes:

    • I do not have the same background as the one in the challenge. The repository that I downloaded does not have a background image included.

    Single Price Grid Component using HTML, Scss, and Bootstrap

    2
    Alex Kim•1,325
    @alex-kim-devPosted about 3 years ago

    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
    1
  • Alex Kim•1,325
    @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 helpful
    1
  • Emmilie Estabillo•5,540
    @emestabilloSubmitted over 4 years ago

    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!

    Mobile-first with Flexbox and old-fashioned CSS

    5
    Alex Kim•1,325
    @alex-kim-devPosted about 4 years ago

    What's everyone's take on the tablet layout for this project?

    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

    1
  • Alex Kim•1,325
    @alex-kim-devPosted about 4 years ago

    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.

    2
  • Alex Kim•1,325
    @alex-kim-devPosted about 4 years ago

    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!

    2
  • Alex Kim•1,325
    @alex-kim-devPosted over 4 years ago

    Hey Mubaraq, great solution! Here are some of my thoughts:

    • I would use aria-label instead of adding a span with .visually-hidden on the buttons, your solution works perfectly though, so that's just a preference;
    • I would add a visually hidden 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!

    3
  • Alex Kim•1,325
    @alex-kim-devPosted over 4 years ago

    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!

    3
  • Alex Kim•1,325
    @alex-kim-devPosted over 4 years ago

    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.

    0