Review 8: Navbar.jsx and AuditedBalanceInput.jsx

Sydnee-You commented 3 weeks ago

Overview

The focus for this code review will be centered around the Navbar and Audited Balance Input pages.
Please pay attention too:

Javascript issues
React components

Review Branch

review-8

Files to review

Checklists

Due date

11/04/24

For more information

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

Sydnee-You commented 3 weeks ago

NavBar.jsx Seems fine. No ESLint errors, comments seem relevent, etc.

AuditedBalanceInput.jsx See what I said above for navbar.

TannerBerry commented 3 weeks ago

NavBar.jsx AR-02 (ln 17): The inline style menuStyle is incorrectly applied to the Navbar component. Instead of using menuStyle directly, it should be passed as style={menuStyle}. DE-04 (ln 20-22): The logo image has a fixed width attribute. Consider making the width dynamic or responsive to enhance the mobile experience.

AuditedBalanceInput.jsx DE-02 (ln 23): Repeated instances of using Col elements with similar attributes. Consider creating a dynamic mapping function to reduce repetition and improve readability.

cjochim commented 3 weeks ago

AuditedBalanceInput.jsx Everything looks okay, well-structured, and easy to read.

Navbar.jsx Everything looks okay as well. However, there's a typo in line 17, menuStyle should be applied as style={menuStyle} instead of inline styling in Navbar.

cash-baker commented 3 weeks ago

Navbar.jsx

Some of the Navlinks use the same key ("key=add"), ensure any keys used are unique to avoid console errors

AuditedBalanceInput.jsx

These buttons use the same styling, might be better to add these to style.css and cut down on duplicate lines of code https://github.com/manoa-inspire/MATP/blob/d09b01b4358045924bae5f8850640c2d13bf7ac7/app/imports/ui/pages/AuditedBalanceInput.jsx#L90-L95

luisy-eng commented 2 weeks ago

Review of `NavBar.jsx`

No ESLint errors, overall looks good

Review of `AuditedBalanceInput.jsx`

Also looks good, no significant changes need to be made.

RomaMalasarte commented 2 weeks ago

NavBar.jsx Design-01, L51-54: Maybe Nav.Link can be refactored or grouped appropriately.

React-06, L16: Avoid inline styles, Move any styles to a CSS file to provide consistency.

AuditedBalanceInput.jsx React-06, L93: Avoid inline styles, move style code into CSS file for consistency

React-05: Perhaps consider using meaningful proptypes

blakewatanabe commented 2 weeks ago

NavBar.jsx

DE-06: Ensure code is readable - The code is readable with no dead code or errors.

AuditedBalanceInput.jsx

DE-07: Ensure code is DRY - inline styles could be moved to the CSS file to remove some of the repeated code. DE-06: Ensure code is readable - The rest of the code is very readable with no errors.

juncell-venzon commented 2 weeks ago

AuditedBalanceInput.js

DE-01: Move inline styles on buttons to CSS classes for consistency and maintainability.

JS-02: The react/jsx-props-no-spreading rule is disabled for spreading props. Consider explicitly passing props to enhance readability or enabling this rule if the spread is essential.

TST-01: Add validation for form fields to ensure users enter appropriate data. This helps prevent errors on form submission.

NavBar.js

JS-01: The variable menuStyle is defined but not used. Remove or use it in the appropriate place.

You can move thee inline styles to CSS and remove unused variables. Also simplifying the conditional rendering for better readability and ensure key props are unique.

manoa-inspire / MATP