Skip to content
OverviewSolutions
6
Comments
10

not-a-patch

@notapatchBirmingham, UK200 points

I’m a mysterious individual who has yet to fill out my bio. One thing’s for certain: I love writing front-end code!

Latest solutions

Latest comments

  • not-a-patch•200
    @notapatch

    You're doing good, Neil.

    It looked feature complete. I'm not very experienced in ReactJS, but I've read a few solutions and your answer was similar to others and the code was clear to read.

    The favourite bit was using Set to create a unique list.

    Array.from(new Set([...attributes, attribute])));

    Nothing more to say other than keep going!

  • not-a-patch•200
    @notapatch

    I must admit, I wouldn't have known if this was a Parcel project or a create-react-app without looking into the package.json. Is there another way?

    I've not used styled components, but I could follow along and nothing was surprising.

    The JS array methods are what I was expecting.

    So the feedback, really is, well done and keep going.

    Marked as helpful
  • not-a-patch•200
    @notapatch

    I've only written 3 ReactJS applications, including this one. I'm quite the newbie, but I could read your code and understand it completely. Very educational, thanks.

    Marked as helpful
  • not-a-patch•200
    @notapatch

    Looks really good. The code looks neat. I have the following issues:

    • The tip percentage doesn't toggle. So when you click on it the button should change color and stay that color. Otherwise, you don't know what percentage you chose when it does the calculation.

    • I don't think it should calculate the tip without you clicking tip. Currently, it does but leaves it as 0.

    • The buttons on the mobile layout should have two columns, not three.

    Good luck and keep going!

  • not-a-patch•200
    @notapatch

    Well that's brilliant @AlexKMarshall

    I've fixed with max-width - I was missing a wrapper around the job list.

    Conditional logic - It's much nicer to have the logic spread over fewer lines - think that's called more cohesion?? Anyway, improvement.

    Div soup. I made a change for the list. Hopefully that's better? If that's right I'll look through the rest of the app.

      <div className="lg:flex-1 lg:justify-end bg-white flex items-center rounded">
        <ul className="mt-5 flex flex-wrap content-start gap-4">
          {jobFilterList.map(filter =>
            <li key={filter} >
              <button
                className="p-2 self-center text-sm font-bold text-primary-200 hover:text-white bg-neutral-400 hover:bg-primary-200 rounded"
                onClick={() => handleAddFilter(filter)}
              >
                {filter}
              </button>
            </li>
          )}
        </ul>
      </div>

    The H3, I mistakenly thought you should only have 1 H1. That isn't the case. I've updated the H3 to H1.

    Anyway, I've put acknowledgement for your code review in my README.me. Thanks, again!

  • not-a-patch•200
    @notapatch

    You've done the hardest bit, version one. But you can make some improvements. With new programmers the most obvious issues you see is a lack of consistency in how they write. Inconsistent code is harder to read. For example:

    Bad - inconsistent, hard to read

         body{
        font-size: 20px;
    }

    .card {
        width: 450px;
        height: 680px;
    }

    .hearder-pic{
        width: 450px;
    }

    Good - consistent, easier to read

       body {
        font-size: 20px;
    }

    .card {
        width: 450px;
        height: 680px;
    }

    .hearder-pic {
        width: 450px;
    }

    You might want to see if you can get a linter to solve these problems. But fixing them by hand for a while is helpful, as learning consistency is a good habit.

    “I'm not a great programmer; I'm just a good programmer with great habits.”

    ― Kent Beck

    Regarding the work. You should try to solve CSS problems like this with Flexbox and flex grid instead of table. Save table for actual rows of data and keep Flexbox and CSS Grid for everything else. The reason is that Flexbox and CSS grid allow you to solve responsive problems easily. Look at this problem, changing between mobile and desktop is literally one line of CSS.

    • It is also worth fixing the 2 HTML issues and the 7 accessibility issues.
    • Consider getting a spell checker in the code, it might have picked up "hearder" which should be header.

    If there's anything you don't understand, I'm happy to write it again.

    Well done and keep going!

View more comments