Review: Dashboard.jsx and Landing.jsx

Sydnee-You commented 4 weeks ago

Overview

The focus for this code review will be centered around the Dashboard and Landing pages.
Please pay attention too:

Javascript issues
React components

Review Branch

review-2

Files to review

Checklists

Due date

09/23/2024 Monday at 9:00am

For more information

The review process is documented at: http://courses.ics.hawaii.edu/ics414s21/morea/review/reading-idpm-review.html

TannerBerry commented 3 weeks ago

Landing.jsx AR-01 (ln N/A): The file name Landing.jsx does not follow the lowercase file name convention. It should be renamed to landing.jsx. DE-06 (ln 9): The comment // eslint-disable-next-line no-unused-vars disables ESLint for the e variable in the handleSelect function, which is unused. This can be removed if e is unnecessary. JS-04 (ln 9): The handleSelect function is already an arrow function, so no changes are required. ES-01 (ln 9): The ESLint rule is disabled for the handleSelect function due to the unused e parameter. Either use e or remove the parameter and the ESLint disable comment. RE-04 (ln N/A): This component does not use props, so no destructuring is required. RE-07 (ln N/A): No data fetching inside render or return. The component behaves as expected.

Dashboard.jsx AR-01 : The file name Dashboard.jsx should follow the lowercase convention. Rename it to dashboard.jsx. DE-01 (ln 82, ln 160, ln 252): Repetitive chart components in Dashboard4yr, Dashboard8yr, and Dashboard12yr should be refactored into reusable components to reduce duplication. JS-04 (ln 6, ln 80, ln 158, ln 250): The renderDashboard function uses an arrow function, which is appropriate. However, other smaller functions like SnapShot, Dashboard4yr, Dashboard8yr, and Dashboard12yr can also be written using arrow functions for consistency. ES-01 (ln N/A): No visible ESLint errors in the displayed code. However, it is recommended to run ESLint to catch potential issues. RE-01 (ln 80, ln 158, ln 250): Components like Dashboard4yr, Dashboard8yr, and Dashboard12yr are quite large and include multiple charts. Consider breaking these into smaller components for maintainability.

cjochim commented 3 weeks ago

Landing.jsx

No duplication and code is clean
comments are useful and informative — perhaps adding more comments like explaining how carousel integrates into landing page
readability is good; structure is organized
Perhaps breaking down components into smaller and more reusable components.

Dashboard.jsx

code duplication: repeated code of the charts for the different dashboards like the structure is the same but only the data changes. We can refactor the repeated elements into reusable components.
large data blocks: we can make a separate file for the data and import them
comments are useful and concise
code is structured and organized well

Sydnee-You commented 3 weeks ago

Dashboard.jsx JS-05 on line 44

Landing.jsx ES-01 and DE-03 on line 12 (parameter e is unused)

RomaMalasarte commented 3 weeks ago

Dashboard.jsx

Line 6-21: Data should be fetched from the API (for now it is hardcoded)

Line 563: move renderDashboard outside of dashboard component to prevent unecessary performance issues.

Line 112, 128, 161, 179, 195, and those similar: To mitigate repeated code for the chart, use LineChart logic as a reusable component.

Landing.jsx

Line 35, 38, 41: There are no explicit accessibility features. Add alt, or aria-label

Line 12: Unused variable e. Use or consider removing it.

blakewatanabe commented 3 weeks ago

DashBoard.jsx

https://github.com/manoa-inspire/MATP/blob/3b6c5e5845544b39eef6c8de67e940303855dc68/app/imports/ui/pages/Dashboard.jsx#L106 Design-07: Instead of inline style, move to css file

Landing.jsx

https://github.com/manoa-inspire/MATP/blob/3b6c5e5845544b39eef6c8de67e940303855dc68/app/imports/ui/pages/Landing.jsx#L21 Design-07: Same here, could move inline style code to css

luisy-eng commented 3 weeks ago

Dashboard.jsx

Modularize Charts: Each chart could be refactored into its reusable component. Data Abstraction: Avoid hardcoding data arrays multiple times. Instead, move them to a data file or API call. Refactor Logic: Look for opportunities to reduce chart setup and rendering duplication.

Landing.jsx

Refactor Feature Cards: The repeated Card structure in the features section can be abstracted into a FeatureCard component to reduce repetition and improve maintainability. Simplify Structure: The component structure is clean, but further refactoring can improve code reuse.

cash-baker commented 3 weeks ago

Dashboard.jsx

Data needs to be fetched from an API, instead of using hard-coded data to construct line graphs. Styling should also be moved to a separate file

Landing.jsx

Might be better to designate styling to a separate style sheet, instead of using inline styles, to improve code readability.

juncell-venzon commented 3 weeks ago

Dashboard.jsx

Adding a reusable button component for the dashboard selection buttons to avoid repeated code. This would make the overall code more easier to read and improve.

Landing.jsx

The carousel lacks alt text for the images that are being used. With this implementation the the image provided from the carousel is more accessible.

manoa-inspire / MATP