Skip to content
OverviewSolutions
9
Comments
3
FunkyCreep
@francoisbillet

All comments

  • FunkyCreep•130
    @francoisbilletSubmitted over 1 year ago

    It's the first time I tried Tailwindcss and I had a hard time deploying my app to Netlify. It turns out I had to commit the dist folder with the CSS file generated by Tailwind to make it work. However, by default this folder is gitignored and I'm not used to versioning this folder since it is supposed to be generated by the build command.

    Expenses Chart Component (React + Tailwindcss + Vite)

    2
    FunkyCreep•130
    @francoisbilletPosted over 1 year ago

    Hi, thank you for your feedback. 1/ We don't need to "import React from 'react'" anymore with React 17+. 2/ The path to data.json works fine since this file is at the root of the project (2 levels down so ../../data.json). 3/ Good point, thank you ! I changed index key to spending.day. 4/ If I remove * 2.5 I will have very small boxes that won't match the design's. On smaller device it's the width that changes, not the height. Have a good day :)

    0
  • FunkyCreep•130
    @francoisbilletPosted over 1 year ago

    Hello, It looks good, congrats !

    A few feedbacks:

    • Use semantic HTML as much as possible, you have a few warnings in your accessibility report about this. For instance in your App.js, you should put a <main> and not a div.
    • In your Results.jsx file your images have an alt property value of "icon" for every single icon. This is not very useful for a person who can't see, who's using a screen reader. Be more explicit about what represents each icon.
    • To get used to building React apps with components, I'd probably have created 2 components: one <Result /> and one <Summary />. I'd also have created a component <CategorySummary /> which would correspond to this bit of code:
    <li key={index} className={styles.category} style={{ color: item.color, background: item.background }}>
       <img src={item.icon} alt="icon" className={styles.images} />
        {item.category}
       <div className={styles.scores}>
            <p className={styles.score}>{item.score}</p> <span>  / 100</span>
       </div>
</li>

    You could then pass props to this components for all the info it has to display. This is a very small project so you don't really need to create smaller components like this but as a project scales it is important to do this to help readability and maintainability.

    About CSS:

    • There is a lot of duplicate values. Imagine if you want to change the general font-size of 18px to 16px. You'd have to change it everywhere. Instead you can declare custom properties in the :root selector (html tag) and use them using var(--my-custom-property). you don't need SCSS to have variables.
    • You should use rem units for font-size (also for margin and padding). Check out the link to know why.

    Anyway, good job ! Keep up the good work :)

    Marked as helpful
    1
  • FunkyCreep•130
    @francoisbilletPosted over 1 year ago

    Hey, good job. It looks pretty good !

    Feedbacks on accessibility:

    • As the accessibility report says, you absolutely need to provide alternate text (alt property) to your images for people who can't see (among others). Also, don't chose your heading tags according to their native styles. There should not be a <h4> if there is no <h3>, there should be no <h3> if there is no <h2> etc. So start with <h1>.

    Feedbacks on React:

    • It's ok to use the Context API to try it out, but React documentation doesn't recommend using it as it complexifies the app we're building. Here you could simply pass props from your App component to your children components.
    • Nonononono ! DON'T DO THIS:
    <div className="upper-portion">
      <h4>Your Result</h4>
      <TotalResult />
      <h2>Great</h2>
      <h4>
        You scored higher than 65% of the people who have taken these tests.
      </h4>
</div>

    Everything isn't a title ! IMO "Your Result" is the title of the page (<h1>), but Great and the text are paragraphs, not titles ! Plus you don't want to put <h2> titles after <h4>, it doesn't make sense.

    • Try to use semantic HTML as much as possible, for instance your button is a <div> but it should be a <button>
    • I'd put a width and height on the circle (instead of using flex-basis) to avoid it changing sizes when we change the screen size.

    I hope this helps. Keep up the good work !

    0