Skip to content
OverviewSolutions
6
Comments
10
not-a-patch
@notapatch

All comments

  • not-a-patch•200
    @notapatchPosted almost 3 years ago

    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!

    1
  • not-a-patch•200
    @notapatchPosted about 3 years ago

    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
    0
  • not-a-patch•200
    @notapatchPosted about 3 years ago

    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
    1
  • not-a-patch•200
    @notapatchPosted about 3 years ago

    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!

    0
  • not-a-patch•200
    @notapatchSubmitted about 3 years ago

    I have no questions, but this is my third ReactJS application, so there's plenty to learn, and I'll take any advice.

    Really enjoying React so far as often the complicated code gets sandboxed into a component. Modern JS if fun when you use the Array methods.

    Job Listings - ReactJS Tailwindcss JS Array Methods

    2
    not-a-patch•200
    @notapatchPosted about 3 years ago

    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!

    0
  • May Sapal Thant•10
    @MaySapalThantSubmitted about 3 years ago

    I would like you all, seniors, to check my answer. How can I do for that answer to be more cooler and effective. My answer isn't nice and even may be wrong. Please I request you kindly check my answer and tell me any wrong things. Thank you so much! Arigato gozaimasu...

    I'm just beginner and I just use table element for positioning.

    1
    not-a-patch•200
    @notapatchPosted about 3 years ago

    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!

    1
  • not-a-patch•200
    @notapatchPosted about 3 years ago

    I have not done this challenge. Your site looks the same, but just offset? I don't think it's important unless you don't know how to make the offset the same.

    I liked:

    • CSS variables.
    • CSS was written consistently - I assume you used a linter or experienced.
    • BEM ? usage (I don't write CSS much now, but as long as you have a system that works for you)

    The only thing I would not be sure about is that usage of colors in naming the cards. My thoughts would be to name it after the type of cars instead because the chances of the car type changing is less than the chance the designer wants to change the colors.

    .card--orange {                                      ===> .card--sedan         ???
  background-color: var(--orange);
  border-top-left-radius: 10px;
  border-top-right-radius: 10px;
}
    Marked as helpful
    0
  • not-a-patch•200
    @notapatchPosted about 3 years ago

    I have a question. When you imported the data in you cached it in a useState hook. Is that standard practice?

    #src/App.js

    import data from './data.json';

function App() {
  const [details, setDetails] = useState(data);  // <=== This bit
  ...
}
    0
  • not-a-patch•200
    @notapatchPosted about 3 years ago

    In your Dashboard.js why did you decide to put the data in Javascript object literals:

    const dailyData = [{ current: 5, previous: 7 },
    { current: 1, previous: 2 },
    { current: 0, previous: 1 },
    { current: 1, previous: 1 },
    { current: 1, previous: 3 },
    { current: 0, previous: 1 },
    ];

    Rather than importing the data in from the JSON file?

    0
  • not-a-patch•200
    @notapatchSubmitted over 3 years ago

    Why are SVGs supplied without the viewbox? This means that you can't resize the SVG. Odd.

    Vite and Tailwindcss

    2
    not-a-patch•200
    @notapatchPosted over 3 years ago

    First off, thank you. People with thousands of experience points are what can make a development site for learning work.

    • I've made the repo public. I'm going for backend jobs and I didn't want loads of frontend repos visible, but one or two won't matter.
    • RE: responsive - I made the site do the desktop and mobile version that was supplied. I noticed it "broke" when moving between them, but left it because nothing was specified, and I'm lazy and already spent longer than I expected. I've seen that website, but didn't get very far - as you might guess, haha. Furthermore, I like feeling the pain points, and then I'll pay better attention during a lesson when I try it again.
    • re: SVG scalability using width - of course you're right, thanks for the tip I was confused.
    • re: lower level challenge. Lower level haha? I've only found one challenge which is responsive and lower level than this. But I take your point, it did stretch me more than I expected (the footer was harder than I thought it was going to be).
    0