[scheduler] Polish EventCalendar rendering for multi-resource events#23161
Conversation
Deploy previewhttps://deploy-preview-23161--material-ui-x.netlify.app/Bundle size
Check out the code infra dashboard for more information about this PR. |
|
Hey @rita-codes, can you review this pr? |
There was a problem hiding this comment.
Thanks for working on this! The coverage matches the visibility scope of #23015 and CI is green, but two of the issue's acceptance criteria aren't fully pinned yet:
-
🟡 The positive assertions don't pin the "renders once" criterion. The acceptance criterion in #23015 is that a multi-resource event renders once in each view, and the characteristic failure mode for multi-resource events is duplicate rendering (one occurrence per assigned resource).
expect(screen.getAllByText('Team Sync').length).to.be.greaterThan(0)still passes in that case, so the tests can't catch the main regression they're meant to guard against. Since these are timed single-day events (the invisible layout placeholders only apply to all-day events), asserting the exact count is safe here. Applies to the new tests in all four views. -
🟡 The color criterion is untested. #23015 also lists "color is derived from the first resource" in its scope, but no test in this PR covers it. The behavior is already implemented (
schedulerEventSelectors.colorresolves through the primary resource id), so it's assertable today — covering it in a single view would be enough, since the selector is shared. Without it, the PR would close the issue with that criterion unpinned. -
🟡 Nit: test descriptions in this repo start with "should ..." — the internals tests added in this PR follow the convention, but the new view tests don't. Worth aligning them for consistency.
Thanks for the detailed review! I've addressed all three points:
The PR has been updated. Could you take another look when you have a chance? Thanks! 🙌 |
rita-codes
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround, all three points are addressed, approving 🙌
One small cleanup: I'd drop the repeated "Pins the 'renders once' criterion…" comments. The test titles already convey it, so the comments don't add information for future readers 👀
Thanks for taking care of this!!
|
Thank you for the review and the approval! 🙌 I've removed those comments as well. Really appreciate all the feedback throughout this PR! |
Closes #23015
Changelog