Review: AuditedBalanceInput and BudgetPlInput.jsx

[blakewatanabe]

Overview

The focus for this code review will be centered around the AuditedBalanceInput page and BudgePlInput page.

Please pay attention too:

• Javascript issues
• React components

Review Branch

review-1

Files to review

• AuditedBalanceInput.jsx
• BudgetPlInput.jsx

Checklists

• Architecture
• Design
• JS
• React

Due date

09/16/24 by 9:00am

For more information

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

[luisy-eng]

Code Review for BudgetPlInput.jsx:

1. File Name:

   • The file name BudgetPlInput.jsx matches the component name.

2. Code Formatting:

   • The JSX indentation is inconsistent in some areas. Please clean up the formatting to make it easier to read and maintain.

3. Inline Styles:

   • Avoid mixing inline styles with external CSS classes. Move styles like backgroundColor and borderColor to a CSS file.

4. Buttons:

   • The submit button currently uses type=" button." Use type="submit" for proper form submission handling.

5. Redundant Wrappers:

   • Some Row and Col elements are nested unnecessarily. Simplify the layout to avoid extra wrapping elements.

6. Improvement Ideas:

   • Add form validation for input fields.
   • Consider extracting repeated form elements into reusable components.

===================

Code Review for AuditedBalanceInput.jsx:

1. File Name:

   • The file name AuditedBalanceInput.jsx matches the component name.

2. Code Formatting:

   • Like the other file, improve JSX indentation for better readability.

3. Inline Styles:

   • Avoid inline styles on buttons. Move these to an external CSS file for consistency.

4. Buttons:

   • The submit button should use type="submit" instead of type="button" for proper form handling.

5. Form Handling:

   • Add form validation and input constraints (e.g., min and max for numeric fields).

6. Simplify Layout:

   • There are some redundant Row and Col wrappers. Remove unnecessary nesting to simplify the code.

[cjochim]

AuditedBalanceInput.jsx

• Redundant {/ Subtitle /} comment, because it seems straightforward that it’s an unnecessary
• Duplication for Form.Group containing Form.Label and Form.Control so refactor to avoid repeated Form.Group components
• Inline styles can be moved to a CSS class

BudgetPIInput.jsx

• Similar to AuditedBalanceInput.jsx, there is redundant unnecessary comment like {/ Subtitle /}
• multiple repeated Form.Group components
• No error handling / form validation for form inputs

[blakewatanabe]

AuditedBalanceInput.jsx

https://github.com/manoa-inspire/MATP/blob/9427a0266b3169ad04bd267793047e8b0b41b635/app/imports/ui/pages/AuditedBalanceInput.jsx#L162 Design-07: Instead of inline style, move to css file

https://github.com/manoa-inspire/MATP/blob/9427a0266b3169ad04bd267793047e8b0b41b635/app/imports/ui/pages/AuditedBalanceInput.jsx#L38 Design-07: Could move repeated classNames to a custom class in css file

BudgetPiInput.jsx

https://github.com/manoa-inspire/MATP/blob/9427a0266b3169ad04bd267793047e8b0b41b635/app/imports/ui/pages/BudgetPlInput.jsx#L143 Design-07: Same here, could move inline style code to css

https://github.com/manoa-inspire/MATP/blob/9427a0266b3169ad04bd267793047e8b0b41b635/app/imports/ui/pages/BudgetPlInput.jsx#L7 Design-03: Remove dead code (div is not being used)

[Sydnee-You]

AuditedBalanceInput.jsx

DE-05 on line 5 (what do you mean by “same format?” Unclear comment. Elaborate a little)

JS-01 on line 10 - file is called “AuditedBalanceInput.jsx” but header 2 says “Audited Balance.”
Maybe change header to say “Audited Balance Input” to keep it consistent?

BudgetPlInput.jsx AR-01: while the file name does use camelcase, might want to consider refractoring BudgetPlInput to BudgetPLInput to make the L more clear.

DE-05 on line 5 (same as the DE-05 on AuditedBalanceInput.jsx)

[TannerBerry]

BudgetPlInput.jsx

Design Checklist Ensure comments are appropriate. Issue: The comment / This is used as a mockup and doesn't require the schema. Will use the same format / should be removed Line: 4

React Checklist Destructure properties in component parameters. Issue: The properties haven’t been destructured yet. Consider destructuring for clarity and readability.

Use arrow functions when appropriate. The functional component is currently written using traditional function syntax could be using arrow function syntax for consistency: Line: 6

AuditedBalanceInput.jsx

Architecture Checklist Obey file name conventions. Issue: Should be renamed to auditedBalanceInput.jsx.

React Checklist Destructure properties in component parameters. Issue: Consider destructuring properties in the component parameters for improved clarity. Use arrow functions when appropriate. The functional component is currently written using traditional function syntax could be using arrow function syntax for consistency: Line: 6

[juncell-venzon]

AuditedBalanceInput.jsx

L8 9: ACCESS-01: Add aria-label or proper accessibility tags to form fields and buttons to improve accessibility for screen readers.

AuditedBalanceInput.jsx L21 27 33 49 60 66 72 78 86 92 98 104 120 126 132: JS-01: Add input validation for the number fields, check if the input is a valid number or not negative.

[RomaMalasarte]

AuditedBalanceInput.jsx: L18-33: DE-01 Condense the use of , reduce duplication File name: AR-01 apply camelCase, should be auditedBalanceInput.jsx

BudgetPllnput.jsx L118-133: DE-01 Condense the use of , reduce duplication

[cash-baker]

AuditedBalanceInput.jsx

(Lines 18-21, 24-27) Code blocks should be refactored to reduce code duplication and improve code readability "Subtitle" comments are redundant, should be removed

BudgetPIInput.jsx

Moving inline styles to external CSS file would help overall code readability and clarity