feat: move show dashboard to mui#967
Open
priscila-moneo wants to merge 3 commits into
Open
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 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)
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"
Author
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
Comment on lines
+30
to
+38
| 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.
Suggested change
| 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.
smarcet
reviewed
Jun 11, 2026
| 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
Author
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
| <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
Signed-off-by: Priscila Moneo <priscila_moneo@hotmail.com.ar>
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ref: https://app.clickup.com/t/9014802374/86babejhm
Summary by CodeRabbit
New Features
Refactor
Style