feat: move show dashboard to mui#967
Conversation
📝 WalkthroughWalkthroughThis PR modernizes the summit dashboard by converting it from a legacy class component to a functional MUI-based layout, adding reusable dashboard components, extending the theme with a new background color, and inserting dashboard section localization strings. ChangesDashboard UI Redesign
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed due to a network error. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pages/summits/summit-dashboard-page.js`:
- Around line 54-58: The useEffect currently calls fetchRegistrationData()
unconditionally on mount causing premature requests before summit context is
ready; update the useEffect (the hook that calls fetchRegistrationData) to only
invoke fetchRegistrationData when currentSummit.id and
currentSummit.time_zone?.name are present—either by adding those values to the
effect dependency array and checking them inside the effect or by gating the
call with an early return inside the effect (referencing useEffect,
fetchRegistrationData, and currentSummit).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3dbeee96-9399-43d7-b662-a19ca0afc281
📒 Files selected for processing (7)
src/components/CustomTheme.jssrc/i18n/en.jsonsrc/pages/summits/components/summit-dashboard-date-range.jssrc/pages/summits/components/summit-dashboard-section-header.jssrc/pages/summits/components/summit-dashboard-stat.jssrc/pages/summits/summit-dashboard-page.jssrc/styles/summit-dashboard-page.less
💤 Files with no reviewable changes (1)
- src/styles/summit-dashboard-page.less
| background: { | ||
| light: "#F7F7F9" | ||
| light: "#F7F7F9", | ||
| secondary: "#eaeaea" |
There was a problem hiding this comment.
these two colors are almost exactly the same, are we sure we want this distinction ?
There was a problem hiding this comment.
in case we do want the two, name it something more meaningful like for example "light_gray"
There was a problem hiding this comment.
The new color added to pallete (secondary) is for this headers
The other one (light) is for pages background
I will improve the name.
| import RemoveIcon from "@mui/icons-material/Remove"; | ||
| import moment from "moment-timezone"; | ||
|
|
||
| function formatDate(ts, tzName) { |
There was a problem hiding this comment.
please use the method in utils/methods.js
| import Box from "@mui/material/Box"; | ||
| import Typography from "@mui/material/Typography"; | ||
|
|
||
| function SummitDashboardSectionHeader({ children }) { |
There was a problem hiding this comment.
This component should be named DashboardSection - since its already inside summit dir , and take two props: title and children , then group everything inside a Box .
| <Card elevation={0}> | ||
| <CardHeader title={T.translate("dashboard.dates")} /> | ||
| <Divider /> | ||
| <SummitDashboardSectionHeader> |
There was a problem hiding this comment.
so this would be a DashboardSection
| tzName={tz} | ||
| /> | ||
| {currentSummit.selection_plans.map((sp) => ( | ||
| <Box key={`sp_${sp.id}`}> |
| </div> | ||
| ); | ||
| } | ||
| <Card elevation={0}> |
There was a problem hiding this comment.
These cards repeat a lot, can we put them in a separate component that takes an array of touples {title, stat} ?
Signed-off-by: Priscila Moneo <priscila_moneo@hotmail.com.ar>
priscila-moneo
force-pushed
the
feature/move-show-dashboard-mui
branch
from
74702da to
0d70957
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/pages/summits/components/summit-dashboard-date-range.js (1)
8-8: 💤 Low valueVerify the double space in
DATE_FORMATis intentional.The format string has two spaces between the date and time components:
"YYYY/MM/DD hh:mm A". If this is unintentional, it should be corrected for consistency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/summits/components/summit-dashboard-date-range.js` at line 8, The DATE_FORMAT constant currently reads "YYYY/MM/DD hh:mm A" and contains a double space between the date and time; verify whether the double space is intentional and if not, change the constant DATE_FORMAT to use a single space ("YYYY/MM/DD hh:mm A") so formatting is consistent across usages (search for DATE_FORMAT in summit-dashboard-date-range.js and update its value accordingly).src/pages/summits/components/dashboard-section.js (1)
20-27: ⚡ Quick winConsider reusing
SummitDashboardSectionHeaderto avoid duplication.Lines 22-24 duplicate the exact header implementation from
SummitDashboardSectionHeader(Box withbackground.gray, padding, and Typography). Consider importing and using that component here instead of reimplementing it.♻️ Proposed refactor to reuse existing component
+import SummitDashboardSectionHeader from "./summit-dashboard-section-header"; function DashboardSection({ title, children, variant }) { if (variant === "card") { return ( <Card elevation={0}> <CardHeader title={title} /> <Divider /> {children} </Card> ); } return ( <Box> - <Box sx={{ bgcolor: "background.gray", px: 2, py: 2 }}> - <Typography variant="body2">{title}</Typography> - </Box> + <SummitDashboardSectionHeader>{title}</SummitDashboardSectionHeader> {children} </Box> ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/summits/components/dashboard-section.js` around lines 20 - 27, The header Box in dashboard-section.js duplicates SummitDashboardSectionHeader; import SummitDashboardSectionHeader and replace the manual Box block (the Box with sx={{ bgcolor: "background.gray", px: 2, py: 2 }} and inner Typography) with <SummitDashboardSectionHeader title={title} /> (or the component's expected prop) so the section uses the shared SummitDashboardSectionHeader component and removes duplicated markup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pages/summits/components/dashboard-section.js`:
- Around line 30-38: DashboardSection.propTypes currently restricts variant to
oneOf(["card"]) while DashboardSection.defaultProps sets variant to undefined,
causing PropTypes warnings; update the prop-type definition for variant to allow
both "card" and undefined (e.g., include null/undefined or make it optional) or
remove the oneOf restriction so runtime behavior matches validation, and ensure
DashboardSection.defaultProps still defines variant as undefined if you want the
non-card default.
In `@src/pages/summits/components/dashboard-stat-section.js`:
- Around line 13-28: The code assumes each group has items which can cause
runtime errors and invalid grid sizes for empty or non-dividing lengths; update
the rows.map callback to skip empty groups (if (group.length === 0) return
null), compute a safe integer column size with something like const size =
Math.max(1, Math.floor(GRID_COLUMNS / group.length)), and derive a robust key
for the fragment using a fallback (e.g., const key = group[0]?.title ||
`group-${i}`) so SummitDashboardStat/Grid2 rendering won't throw or produce
Infinity/non-integer sizes.
---
Nitpick comments:
In `@src/pages/summits/components/dashboard-section.js`:
- Around line 20-27: The header Box in dashboard-section.js duplicates
SummitDashboardSectionHeader; import SummitDashboardSectionHeader and replace
the manual Box block (the Box with sx={{ bgcolor: "background.gray", px: 2, py:
2 }} and inner Typography) with <SummitDashboardSectionHeader title={title} />
(or the component's expected prop) so the section uses the shared
SummitDashboardSectionHeader component and removes duplicated markup.
In `@src/pages/summits/components/summit-dashboard-date-range.js`:
- Line 8: The DATE_FORMAT constant currently reads "YYYY/MM/DD hh:mm A" and
contains a double space between the date and time; verify whether the double
space is intentional and if not, change the constant DATE_FORMAT to use a single
space ("YYYY/MM/DD hh:mm A") so formatting is consistent across usages (search
for DATE_FORMAT in summit-dashboard-date-range.js and update its value
accordingly).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 59f218f7-3df4-4e70-849c-f2fd109b2b1e
📒 Files selected for processing (9)
src/components/CustomTheme.jssrc/i18n/en.jsonsrc/pages/summits/components/dashboard-section.jssrc/pages/summits/components/dashboard-stat-section.jssrc/pages/summits/components/summit-dashboard-date-range.jssrc/pages/summits/components/summit-dashboard-section-header.jssrc/pages/summits/components/summit-dashboard-stat.jssrc/pages/summits/summit-dashboard-page.jssrc/styles/summit-dashboard-page.less
💤 Files with no reviewable changes (1)
- src/styles/summit-dashboard-page.less
✅ Files skipped from review due to trivial changes (1)
- src/i18n/en.json
🚧 Files skipped from review as they are similar to previous changes (3)
- src/pages/summits/components/summit-dashboard-section-header.js
- src/pages/summits/components/summit-dashboard-stat.js
- src/pages/summits/summit-dashboard-page.js
| DashboardSection.propTypes = { | ||
| title: PropTypes.string.isRequired, | ||
| children: PropTypes.node.isRequired, | ||
| variant: PropTypes.oneOf(["card"]) | ||
| }; | ||
|
|
||
| DashboardSection.defaultProps = { | ||
| variant: undefined | ||
| }; |
There was a problem hiding this comment.
Fix PropTypes inconsistency for variant.
Line 33 restricts variant to oneOf(["card"]), but Line 37 defaults it to undefined. This creates a PropTypes validation warning since undefined is not in the allowed values. The component logic (Line 10) correctly handles undefined by rendering the non-card variant.
🐛 Proposed fix
Option 1: Remove the oneOf restriction if only "card" is needed as an explicit signal:
DashboardSection.propTypes = {
title: PropTypes.string.isRequired,
children: PropTypes.node.isRequired,
- variant: PropTypes.oneOf(["card"])
+ variant: PropTypes.string
};Option 2: Make the PropTypes match the runtime behavior by allowing both "card" and undefined:
DashboardSection.propTypes = {
title: PropTypes.string.isRequired,
children: PropTypes.node.isRequired,
- variant: PropTypes.oneOf(["card"])
+ variant: PropTypes.oneOf(["card", undefined])
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| DashboardSection.propTypes = { | |
| title: PropTypes.string.isRequired, | |
| children: PropTypes.node.isRequired, | |
| variant: PropTypes.oneOf(["card"]) | |
| }; | |
| DashboardSection.defaultProps = { | |
| variant: undefined | |
| }; | |
| DashboardSection.propTypes = { | |
| title: PropTypes.string.isRequired, | |
| children: PropTypes.node.isRequired, | |
| variant: PropTypes.string | |
| }; | |
| DashboardSection.defaultProps = { | |
| variant: undefined | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pages/summits/components/dashboard-section.js` around lines 30 - 38,
DashboardSection.propTypes currently restricts variant to oneOf(["card"]) while
DashboardSection.defaultProps sets variant to undefined, causing PropTypes
warnings; update the prop-type definition for variant to allow both "card" and
undefined (e.g., include null/undefined or make it optional) or remove the oneOf
restriction so runtime behavior matches validation, and ensure
DashboardSection.defaultProps still defines variant as undefined if you want the
non-card default.
| background: { | ||
| light: "#F7F7F9" | ||
| light: "#F7F7F9", | ||
| gray: "#eaeaea" |
There was a problem hiding this comment.
@priscila-moneo please rename this to the proposed name "light_gray"
| import RemoveIcon from "@mui/icons-material/Remove"; | ||
| import { parseAndFormat } from "../../../utils/methods"; | ||
|
|
||
| const DATE_FORMAT = "YYYY/MM/DD hh:mm A"; |
There was a problem hiding this comment.
this is not standard US date format, and constants should go in constants.js
There was a problem hiding this comment.
Ok, format is following Figma design:
| import Box from "@mui/material/Box"; | ||
| import Typography from "@mui/material/Typography"; | ||
|
|
||
| function SummitDashboardSectionHeader({ children }) { |
| }; | ||
| this.onCollapseChange = this.onCollapseChange.bind(this); | ||
| } | ||
| if (!currentSummit.id || !currentSummit.time_zone?.name) return null; |
There was a problem hiding this comment.
So if timezone is not set you won't show the entire dashboard? seems too harsh
There was a problem hiding this comment.
Also check on currentSummit.id is redundant since its already guarded in SummitIdLayout
Signed-off-by: Priscila Moneo <priscila_moneo@hotmail.com.ar>
priscila-moneo
force-pushed
the
feature/move-show-dashboard-mui
branch
from
0d70957 to
2a28aa4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pages/summits/components/summit-dashboard-date-range.js`:
- Around line 27-31: The component calls parseAndFormat(String(startTs), "X",
DATETIME_FORMAT, "UTC", tzName) with tzName possibly undefined which causes
moment-timezone to treat .tz(undefined) as a getter; update the call site in the
SummitDashboardDateRange component (or alternatively inside parseAndFormat) to
normalize tzName to "UTC" when it is missing but not coerce valid falsy values
like 0 — e.g. compute a safeTz = (tzName === undefined ? "UTC" : tzName) and
pass safeTz to parseAndFormat so moment-timezone receives an explicit timezone
string.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 038d3385-2170-4db2-b78a-0e701bdf29cf
📒 Files selected for processing (5)
src/components/CustomTheme.jssrc/pages/summits/components/dashboard-section.jssrc/pages/summits/components/dashboard-stat-section.jssrc/pages/summits/components/summit-dashboard-date-range.jssrc/pages/summits/summit-dashboard-page.js
🚧 Files skipped from review as they are similar to previous changes (4)
- src/pages/summits/components/dashboard-stat-section.js
- src/components/CustomTheme.js
- src/pages/summits/summit-dashboard-page.js
- src/pages/summits/components/dashboard-section.js
| {parseAndFormat(String(startTs), "X", DATETIME_FORMAT, "UTC", tzName)} | ||
| </Typography> | ||
| <RemoveIcon sx={{ mx: 2, fontSize: 16 }} /> | ||
| <Typography variant="body1"> | ||
| {parseAndFormat(String(endTs), "X", DATETIME_FORMAT, "UTC", tzName)} |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In moment-timezone 0.5.33, what is the behavior of momentObj.tz(undefined)? Does it return a moment instance or act like the getter overload?
💡 Result:
In moment-timezone 0.5.33, calling momentObj.tz(undefined) acts as the getter for the timezone [1][2]. It does not return a new moment instance; instead, it returns the timezone name (a string) assigned to the moment instance, or undefined if no timezone has been set [1][2][3]. When the.tz method is called with no arguments or with undefined, it functions as a getter, allowing you to retrieve the current timezone associated with that specific moment object [1][2][3]. This is distinct from the setter behavior, which occurs when a string (timezone name) is passed as the first argument, in which case the method returns the moment instance itself to allow for method chaining [1][4].
Citations:
- 1: https://github.com/moment/momentjs.com/blob/master/docs/moment-timezone/01-using-timezones/03-converting-to-zone.md
- 2: https://momentjs.com/timezone/docs/
- 3: Getting time zone from moment is not documented moment/momentjs.com#353
- 4: https://momentjscom.readthedocs.io/en/latest/moment-timezone/01-using-timezones/03-converting-to-zone/
Fix timezone handling when tzName is omitted
tzName defaults to undefined, but parseAndFormat is still called with it. In moment-timezone, momentObj.tz(undefined) uses the getter overload (returns a timezone name / undefined) rather than setting a timezone, so this can lead to incorrect formatting and potential runtime issues depending on how parseAndFormat uses the result.
Update the component (or parseAndFormat) to normalize tzName to "UTC" when it’s missing (and keep the guard from treating 0 as “empty”).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pages/summits/components/summit-dashboard-date-range.js` around lines 27
- 31, The component calls parseAndFormat(String(startTs), "X", DATETIME_FORMAT,
"UTC", tzName) with tzName possibly undefined which causes moment-timezone to
treat .tz(undefined) as a getter; update the call site in the
SummitDashboardDateRange component (or alternatively inside parseAndFormat) to
normalize tzName to "UTC" when it is missing but not coerce valid falsy values
like 0 — e.g. compute a safeTz = (tzName === undefined ? "UTC" : tzName) and
pass safeTz to parseAndFormat so moment-timezone receives an explicit timezone
string.
| <Typography variant="body2">{label}</Typography> | ||
| </Box> | ||
| <Typography variant="body1"> | ||
| {parseAndFormat(String(startTs), "X", DATETIME_FORMAT, "UTC", tzName)} |
There was a problem hiding this comment.
this method is intended to be used with a string date like "12/12/1983" , you have a number that is epoch . You should be using something like formatDate in utils/methods or epochToMomentTimezone in uicore
3 participants
ref: https://app.clickup.com/t/9014802374/86babejhm
Summary by CodeRabbit
New Features
Refactor
Style