Shravya_bugfix/css_to_module.css_rentalCharts_3552#4092
Conversation
✅ Deploy Preview for highestgoodnetwork-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
ShravyaKudlu
force-pushed
the
Shravya/bugfix/rentalcost_3552
branch
from
713c949 to
ddf329a
Compare
ShravyaKudlu
added
the
High Priority - Please Review First
somramnani
left a comment
somramnani
left a comment
There was a problem hiding this comment.
I tested this locally using an admin account and followed the provided steps.
The RentalChart graph renders correctly, all functions work as expected, and inline styles have been replaced with the new RentalChart.module.css. Verified that the chart and components display properly in dark mode.
Issue observed:
In light mode, all graph items including table headers, dates, and titles are not visible.
Dark Mode (working as expected):
Light Mode (issue):
MeronTeweldebrhan
left a comment
MeronTeweldebrhan
left a comment
There was a problem hiding this comment.
**PR Review Feedback
Testing Outcome**
-
After navigating to /bmdashboard/rentalchart, I was redirected to the login page. Submitting admin credentials did not respond immediately the page hung until a manual reload, which then loaded the correct path but showed console warnings.
-
Dark and light mode functionality works; however, the header “Rental Cost Over Time” has poor readability in dark mode and does not meet WCAG accessibility standards.
🔎 Functionality verified, with noted issues above for improvement.
There was a problem hiding this comment.
I tested PR and found most everything is working as expected. All inline styles are removed from RentalChart.js and moved to RentalChart.module.css.
- In dark mode things look great!
- In light mode we dont see the total cost or the months at the bottom.
- I added in a quick fix you may want to try below, I added in a ternary for font color on line 251 simialr to whats being done for bgcolor. Added a ternary to titleColor as well:
RitzzzZ2021
left a comment
RitzzzZ2021
left a comment
There was a problem hiding this comment.
The migration from CSS to CSS modules looks good in dark mode. The chart responds well to user interactions and clearly displays the data. All filters function as expected.
In light mode, the title and labels are not displayed, likely due to the title and labels being hard-coded in white. I see Nhujarski already gave a helpful solution!
There was a problem hiding this comment.
- Add darkMode check for textColor as well so that font color changes when switching to light mode.
Aditya-gam
left a comment
Aditya-gam
left a comment
There was a problem hiding this comment.
-
Dark Mode:
The chart renders correctly, but the header text “Rental Cost Over Time” has poor readability against the dark background. It does not meet WCAG accessibility contrast standards.
-
Light Mode:
In light mode, most visual elements, including chart items, table headers, dates, and titles, are not visible, which makes the chart unreadable.
-
Functionality:
Aside from the visual and accessibility issues, the chart displays correctly, the filtering works as expected, and there are no errors in the graph rendering or behavior.
TestVideo.mov
Core functionality works as intended; however, visual issues in both dark and light modes need to be addressed to ensure readability and accessibility.
|
✅ What works well The Rental Chart page is responsive across different screen sizes. Charts adapt well to both dark and light modes, with suitable colors for readability. The chart does not plot correctly when selecting a start and end date. It seems the start or end month is not being Light mode Dark mode Incorrect start/end date plotting |
akshith312
left a comment
akshith312
left a comment
There was a problem hiding this comment.
Tested Locally.
Logged in as Admin.
The charts render correctly and all the filters work properly in both light and dark modes.
|
Tested the PR, everything works as described. |
|
Reviewed PR #4092
|
aryanrachala54
left a comment
aryanrachala54
left a comment
There was a problem hiding this comment.
Tested the PR locally and everything works as expected.
- The charts render correctly in both light and dark modes.
- All filters function properly, and the data updates as intended.
- The transition from inline styles to
RentalChart.module.csslooks clean and effective. - No layout issues or console errors observed.
laynet
left a comment
laynet
left a comment
There was a problem hiding this comment.
I'm getting this error when I try to checkout this branch
sircarmart
left a comment
sircarmart
left a comment
There was a problem hiding this comment.
Styling still works and all inline styling was appropriately moved into the module css. Looks good in both light/dark modes
Aswin20010
left a comment
Aswin20010
left a comment
There was a problem hiding this comment.
Reviewed PR #4092 (Rental Chart Styling Refactor). Verified migration from inline styles to RentalChart.module.css and confirmed proper styling consistency across light and dark themes. Checked graph rendering under /bmdashboard/rentalchart to ensure functionality and visual alignment remained intact. No UI or functional regressions found — approved the PR.
rohanrastogi311
left a comment
rohanrastogi311
left a comment
There was a problem hiding this comment.
Hi Shravya,
Well done, implementation is working well and smoothly.
ShravyaKudlu
dismissed stale reviews from rohanrastogi311, abdel-lall, and akshith312
via
ebc45b9
|
|
17 participants
Description
Fixes #3552
332 PR BM Dashboard – Add .module.css for /BMDashboard/RentalChart/RentalChart
Related PRS (if any):
This frontend PR is related to the development branch.
Main changes explained:
How to test:
npm installand...to run this PR locallyhttp://localhost:5173/bmdashboard/rentalchartScreenshots or videos of changes:
Note:
The light mode and dark mode is the one you see on official prod website, I haven't changed the default colors.