feat: migrate event category list/edit to MUI components - #1024
Open
tomrndom wants to merge 3 commits into
Open
Conversation
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughEvent category forms and listing pages were migrated to functional React components using hooks, Formik, Yup, and MUI. Icon uploads, subtrack management, scheduling controls, category copying, sorting, localization, reducer mappings, and page action wiring were updated. ChangesEvent category management
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 4
🧹 Nitpick comments (1)
src/components/forms/event-category-form.js (1)
387-420: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueConsider coercing
proposed_schedule_transition_timebefore submit.It initializes to
""and MUI number inputs yield strings, so the payload sends""rather thannull/omitted when unset. Also, since the accordion is collapsed by default, a server error on this field won't be visible (anduseScrollToErrorcan't locate the hidden input) — auto-expanding when the field has an error would help.🤖 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/components/forms/event-category-form.js` around lines 387 - 420, The event form submit flow should normalize proposed_schedule_transition_time from its initial empty string/string input into the API’s expected null or omitted value when unset, while preserving numeric values when provided. In the form component, also tie scheduleSettingsExpanded to validation errors for this field so the accordion automatically expands when proposed_schedule_transition_time has an error, allowing useScrollToError to locate the input.
🤖 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/components/forms/event-category-form.js`:
- Around line 461-463: Update the MuiTableSortable usage in the event category
form to ensure its data prop receives a safe empty collection when
entity.subtracks is undefined, while preserving existing subtrack data when
available.
In `@src/pages/events/edit-event-category-page.js`:
- Around line 45-61: Update the edit-event-category page so the “Add new” Button
is rendered only when entity.id exists, rather than relying on handleAddNew’s
early return; also replace the literal button label with the appropriate
T.translate key for the add-new action.
In `@src/pages/events/event-category-list-page.js`:
- Around line 53-57: Update the render guard in the component around the
currentSummit useEffect to handle an undefined currentSummit consistently with
the existing optional chaining. Check currentSummit safely before accessing its
id, returning null when the summit is absent.
- Around line 33-39: Update the `color` column’s `render` function to display
`row.color` as a visual swatch instead of using `dangerouslySetInnerHTML`. Reuse
the swatch rendering approach from `subtrackColumns` in
`event-category-form.js`, while preserving the existing color value and column
behavior.
---
Nitpick comments:
In `@src/components/forms/event-category-form.js`:
- Around line 387-420: The event form submit flow should normalize
proposed_schedule_transition_time from its initial empty string/string input
into the API’s expected null or omitted value when unset, while preserving
numeric values when provided. In the form component, also tie
scheduleSettingsExpanded to validation errors for this field so the accordion
automatically expands when proposed_schedule_transition_time has an error,
allowing useScrollToError to locate the input.
🪄 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: 149cc38d-394c-4ac8-8254-62329888480c
📒 Files selected for processing (5)
src/components/forms/event-category-form.jssrc/i18n/en.jsonsrc/pages/events/edit-event-category-page.jssrc/pages/events/event-category-list-page.jssrc/styles/edit-event-category-page.less
💤 Files with no reviewable changes (1)
- src/styles/edit-event-category-page.less
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/pages/events/event-category-list-page.js (2)
62-64: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard the fetch effect by
currentSummit?.id.
getEventCategoriesdereferencescurrentSummit.id;if (currentSummit)still invokes it for an object without an ID, potentially requesting/summits/undefined/tracks.Proposed fix
useEffect(() => { - if (currentSummit) getEventCategories(); + if (currentSummit?.id) getEventCategories(); }, [currentSummit?.id]);🤖 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/events/event-category-list-page.js` around lines 62 - 64, Update the useEffect containing getEventCategories to guard the fetch with currentSummit?.id rather than only currentSummit, ensuring getEventCategories runs only when a valid summit ID exists while preserving the current dependency on currentSummit?.id.
81-84: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReplace
m3with a supported breakpoint in bothGrid2containers.m3isn’t defined in the theme, so these sizes will be ignored;mdlooks like the intended value.🤖 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/events/event-category-list-page.js` around lines 81 - 84, Replace the unsupported m3 breakpoint with md in both Grid2 containers in src/pages/events/event-category-list-page.js at lines 81-84 and 103-106, preserving the existing xs sizing and spacing.
🧹 Nitpick comments (1)
src/pages/events/event-category-list-page.js (1)
29-35: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winKeep translated headers reactive.
T.translate(...)now runs once at module evaluation. If the locale changes at runtime or translations load asynchronously, these headers remain stale. Keepcolumnsinside the component or memoize it from the active locale.🤖 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/events/event-category-list-page.js` around lines 29 - 35, Update the columns definition used by the event category list component so translated headers are recomputed when the active locale or translations change. Move columns into the component that renders them, or memoize it using the locale/translation dependency, while preserving the existing column keys and translation keys.
🤖 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/reducers/events/event-category-list-reducer.js`:
- Around line 63-75: Update the EVENT_CATEGORIES_SEEDED handler to preserve the
complete event category shape by mapping the same fields as
RECEIVE_EVENT_CATEGORIES, including code, color, and ordering data alongside id
and name. Keep the existing append and empty-result behavior unchanged.
---
Outside diff comments:
In `@src/pages/events/event-category-list-page.js`:
- Around line 62-64: Update the useEffect containing getEventCategories to guard
the fetch with currentSummit?.id rather than only currentSummit, ensuring
getEventCategories runs only when a valid summit ID exists while preserving the
current dependency on currentSummit?.id.
- Around line 81-84: Replace the unsupported m3 breakpoint with md in both Grid2
containers in src/pages/events/event-category-list-page.js at lines 81-84 and
103-106, preserving the existing xs sizing and spacing.
---
Nitpick comments:
In `@src/pages/events/event-category-list-page.js`:
- Around line 29-35: Update the columns definition used by the event category
list component so translated headers are recomputed when the active locale or
translations change. Move columns into the component that renders them, or
memoize it using the locale/translation dependency, while preserving the
existing column keys and translation keys.
🪄 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: d125a2b3-cb1a-4d41-b6d6-7313a295728c
📒 Files selected for processing (5)
src/components/forms/event-category-form.jssrc/i18n/en.jsonsrc/pages/events/edit-event-category-page.jssrc/pages/events/event-category-list-page.jssrc/reducers/events/event-category-list-reducer.js
🚧 Files skipped from review as they are similar to previous changes (3)
- src/i18n/en.json
- src/pages/events/edit-event-category-page.js
- src/components/forms/event-category-form.js
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
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
1 participant
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/86bb2f7kf
Signed-off-by: Tomás Castillo tcastilloboireau@gmail.com
Summary by CodeRabbit