ScottishCovidResponse / rampvis-ui

This is a React based RAMP VIS visualisation interface. This client uses the REST API: https://github.com/ScottishCovidResponse/rampvis-api
BSD 2-Clause "Simplified" License
7 stars 6 forks source link

Feature ensemble nextjs #109

Closed kamakshidasan closed 2 years ago

saifulkhan commented 2 years ago

Thanks, @kamakshidasan for creating the pull request. I checked it quickly and please find my comments below.

  1. Fix the GitHub code check/linting to address the "All checks have failed"
  2. Move your components to the components folder. See this commit as an example https://github.com/ScottishCovidResponse/rampvis-ui/commit/8848fa5dce99042a93b670e3b6e05d1d6fdcf6a1 After that run yarn build to check if this command passes.

Hi @phongvis, I can help you with reviewing the code, whenever you are free in the evenings.

kamakshidasan commented 2 years ago

@saifulkhan thanks for your comment. I seem to have fixed most of the issues.

The current check seems to fail on yarn-deduplicate --fail --list --strategy=fewer

Can you please advice what to do?

saifulkhan commented 2 years ago

Not 100% sure, but review the changes in yarn.lock file, each change should make sense, and remove any duplicate / package entry.

phongvis commented 2 years ago

I see the React files are well organised under emsemble folder. Other more global changes look reasonable as well. So, probably no crashes will happen :). I don't think it's necessary for me to check details in each React file. After the lint is fixed, I'm happy to merge.

saifulkhan commented 2 years ago

Hi @kamakshidasan, Phong and I reviewed your PR.

  1. You need to fix the error, which is due to the change in yarn.lock file as you added a package named styled component. You can run npx yarn-deduplicate --fail --list --strategy=fewer to check. We don't know how to fix this. Probably you can message Alex.

  2. Could you please explain the change in the file .eslistrc

    "@typescript-eslint/no-this-alias": [
      "error",
      {
        "allowDestructuring": true,
        "allowedNames": ["_this"]
      }
    ]
  3. We do not have any comments. If you address the above comments we shall merge your code.

kamakshidasan commented 2 years ago

Thanks for reviewing my code.

The second point refers to this line: https://github.com/ScottishCovidResponse/rampvis-ui/blob/feature-ensemble-nextjs/src/components/ensemble/controller.js#L97

I need to access "this" inside another one where scope is not present

phongvis commented 2 years ago

@kamakshidasan you could consider using arrow function. I think it doesn't its own this and you can still access to the outside this. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Arrow_functions.

Regarding the first point, please do some research and contact Alex for help as he is an expert in React/JS.