review-02: ClientDataImport.jsx and FileInput.jsx

vavalverde commented 1 month ago

Overview

In a separate document create a list in accordance with the checklist of issues you find. It may be helpful to link the code and include the checklist section it falls under. This is due Monday preferably at the start of our designated class time. Do not post the comment before then!

Example

Review Branch

review-02

Files to review

ClientDataImport.jsx FileInput.jsx

Checklists

Design JS

Due date

9/23/24

For more information The review process is documented at: http://courses.ics.hawaii.edu/ics414f24/morea/review/reading-idpm-review.html

vavalverde commented 1 month ago

ClientDataImport.jsx

Line 9 | JS-08 description can be made into JSDoc https://github.com/visualizing-your-future/visualizing-your-future/blob/a7529902b026a223935d40751dee0bbbc4f320a9/app/imports/ui/pages/ClientDataImport.jsx#L9

Line 20 | JS-01 Stuffs could be more descriptive https://github.com/visualizing-your-future/visualizing-your-future/blob/a7529902b026a223935d40751dee0bbbc4f320a9/app/imports/ui/pages/ClientDataImport.jsx#L20

Line 17 | Design-05 comment probably not needed https://github.com/visualizing-your-future/visualizing-your-future/blob/a7529902b026a223935d40751dee0bbbc4f320a9/app/imports/ui/pages/ClientDataImport.jsx#L17

ESLINT-01-02 No errors currently

FileInput.jsx

JS-01 Name could be ExcelFileUploader but probably not necessary https://github.com/visualizing-your-future/visualizing-your-future/blob/a7529902b026a223935d40751dee0bbbc4f320a9/app/imports/ui/components/FileInput.jsx#L6

Line 14 | JS-04 can destructure line https://github.com/visualizing-your-future/visualizing-your-future/blob/a7529902b026a223935d40751dee0bbbc4f320a9/app/imports/ui/components/FileInput.jsx#L14

Line 23 | JS-01 names can be more descriptive https://github.com/visualizing-your-future/visualizing-your-future/blob/a7529902b026a223935d40751dee0bbbc4f320a9/app/imports/ui/components/FileInput.jsx#L23

Design-08 the padding can be moved to css https://github.com/visualizing-your-future/visualizing-your-future/blob/a7529902b026a223935d40751dee0bbbc4f320a9/app/imports/ui/components/FileInput.jsx#L72 https://github.com/visualizing-your-future/visualizing-your-future/blob/a7529902b026a223935d40751dee0bbbc4f320a9/app/imports/ui/components/FileInput.jsx#L86

Line 13 | JS-08 can use JSDoc comment to describe expected functionality https://github.com/visualizing-your-future/visualizing-your-future/blob/a7529902b026a223935d40751dee0bbbc4f320a9/app/imports/ui/components/FileInput.jsx#L13

ESLINT-01-02 No errors current

bri111 commented 1 month ago

FileInput.jsx

Please add more comments.

I think style should be more in style sheet incase we need to call it somewhere else in the website.

ClientDataImport

L13-15 use /**/ instead of // since it’s multiline

janeljo commented 1 month ago

ClientDataImport.jsx

“Design-03: Eliminate dead code.” Are the references to the StuffCollection used in this file?

https://github.com/visualizing-your-future/visualizing-your-future/blob/a7529902b026a223935d40751dee0bbbc4f320a9/app/imports/ui/pages/ClientDataImport.jsx#L16

FileInput.jsx

“JS-01: Name constructs appropriately.” It might be clearer to rename ‘wb’ as ‘workbook’ to be more descriptive.

https://github.com/visualizing-your-future/visualizing-your-future/blob/a7529902b026a223935d40751dee0bbbc4f320a9/app/imports/ui/components/FileInput.jsx#L19

DavidRickards commented 1 month ago

Review Issue - 02

ClientDataImport.jsx

Comments:

You could combine the comments in 13, 14, and 15 in one shorter line.

Naming:

More concerned with the naming other packages.
Choose more detailed replacement for “Stuff” as it is generic and broad
Is Stuffcollection recent? Or is it old and part of something that needs to be changed and updated?

FileInput.jsx

Design-08: Ensure code is DRY.

The sheet_to_json logic inside loadSheetData could be more flexible. You are repeating some small patterns, like mapping through the data.

Design-05: Ensure comments are appropriate.

There is actually more of a lack of comments in this file which is not a bad thing, “// Get sheet names” & “// Get workbook” maybe readable enough and not necessary to add in
I would think about adding a comment briefly explaining what lines 13-30 do.

johnserraon commented 1 month ago

Review 02

ClientDataImport.jsx

Design-05: Line 9 comment should be updated to remove references of Stuff. It could also be modified into a JS-Doc comment to underline its documentation of the file, e.g.:

/**
* Renders a table containing data from the customer's file.
* FileInput stores the user's input data
* Returns a container displaying the data from FileInput
*/

FileInput.jsx

Design-05: Would be helpful to see comments at each function for more code transparency (i.e. Lines 13, 32, 37). Would also be good to expand on JS-Doc comment.

GalenChang commented 1 month ago

ClientDataImport.jsx

Comments

I am assuming this page is a work in progress.
Lots of references to "Stuffs" and "StuffCollection", which is from the template.
Page code is incomplete.

JS-08

Wrong comment format
- Used //, should be /* comment /

Design-05

Comments are not descriptive or redundant. For example:
- // Determine if the subscription is ready const rdy = subscription.ready();

FileInput.jsx

Design-05

Code is readable, but could use comments describing what each section of code is doing to simplify understanding.

Line 17 - JS-01

Variable "e" could be more descriptive.

Line 19 - JS-01

Variable "wb" could be more descriptive.

visualizing-your-future / visualizing-your-future