Review 9: ImportSheet.jsx and ManageProjections.jsx

PaytonHAH commented 1 week ago

Overview

Please pay attention too:

Javascript issues
React components

Review Branch

review-9

Files to review

Checklists

Due date

Monday, 11/11/2024

For more information

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

t-tirabassi commented 1 week ago

ImportSheet.jsx

Design-05: The comments spread throughout the code could be removed and instead replaced with comments that explain the functionalities of each section of the code.

Design-07/09: The constants could be moved into utilities for reusability and readability.

REACT-01: The code for the page could be broken up into different components.

ManageProjections.jsx

ESLINT-01: The eslint disables should be removed and the code should be refactored to avoid this.

Design-05: The small comments throughout the code could be condensed into singular explanations.

caspascual commented 3 days ago

ImportSheet.jsx

L31-33: Design-09: Move into separate file in utilities for data processing

L35-53: Design-09: Move into separate file in utilities for data processing

L58-79: Design-09: Move into separate file in utilities for data processing

L81-240: Design-09: Move into separate file in utilities for data processing

L242-267: Design-09: Move into separate file in utilities for file processing

L269-272: Design-09: Move into separate file in utilities for file processing

L273-276: Design-09: Move into separate file in utilities for file processing

L280-286: Design-09: Move into separate file in utilities for file processing

JS-01: Rename unnecessaryDataRemoved to something that is not unnecessarily long

JS-07: Remove all console logs

ManageProjections.jsx

L35-40: Design-09: Move into separate file in utilities for file processing

L42-67: Design-09: Move into separate file in utilities for file processing

L69-86: Design-09: Move into separate file in utilities for file processing

L30-31: Can remove, data and setData is used for showing the uploaded data in ImportSheet.jsx using react-spreadsheet

beydlern commented 3 days ago

ImportSheet.jsx

L5: Wildcard import should be reassessed to see if you're using everything from xlsx.
The console.log() statements should be commented or removed.

ManageProjections.jsx

L149-155: "Loading" text can be used as input inside the LoadingSpinner component using the input message (e.g. \).

samallari commented 2 days ago

ImportSheet.jsx

L87-235: Is there a way to refactor unnecessaryDataRemoved to avoid hard coding it for each category and index? Like using a for loop?

ManageProjections.jsx

L30-31: data and setData was copied over from ImportSheet.jsx, can remove and this will fix eslint error

bksnelson commented 2 days ago

ImportSheet.jsx JS-07 avoid using console log. React-01 can be used as component. ManageProjections.jsx JS-07 avoid using console log.

PaytonHAH commented 1 day ago

ImportSheet.jsx

JS-07: Avoid console.logs Lines: 89, 99, 100, 117, 118, 135, 136, 150, 151, 164, 165, 179, 180, 188, 189, 206, 207, 228, 229, 238, 239

ManageProjections.jsx

ESLINT-01: No errors, avoid warnings. Disabling of ESlint should be avoided

Team-Half-and-Half / spirebooks