[scheduler] enable multi-resource event creation and editing - #23313
Conversation
Deploy previewBundle size
Check out the code infra dashboard for more information about this PR. |
|
Hey @rita-codes, can I get a review on this PR? |
rita-codes
added
type: new feature
PR reviewThe core fix is right: the old form seeded Bugs (3)1. 🔴 Saving through the dialog rewrites
|
|
Design decision: how single vs multiple resource selection is resolved @mustafajw07 — after discussing this with @flaviendelangle we've settled on the model below. It supersedes finding 1 in my review above (the one about the save path rewriting 1. New prop: A new boolean on the existing event creation config ( 2. Editing follows the data When editing an existing event, the mode is not decided by the prop — it comes from that event's own
Both shapes may coexist in the same dataset, and we never convert one into the other. An event that arrived as 3. Event whose These are the only two values that carry no shape to infer from, so they're the ones that fall back to 4. Inferred from the data. Scan the events in order until one is found whose This inference only decides event creation and the point 3 fallback. It is never applied per event: existing events always follow their own value per point 2. It also covers the cases where there is no creation config to read the prop from — What this means for the write path
The important part is the invariant: saving an event never changes the shape of its Thanks for bearing with us while we settled this one 🙇♀️ |
|
Thanks for the detailed review and for clarifying the single vs. multiple resource behavior. I've addressed the review feedback:
Ready for another review. |
PR reviewThank you for the work on this one — we took a design decision that landed mid-review and rebuilt the whole resolution model around it, and the result is faithful to it. The shape of Verified on the branch: One thing is merge-blocking, and it's a case we under-specified rather than something you got wrong: on the Event Timeline, Bugs (2)1. 🔴 On the Event Timeline,
|
|
Thanks for the detailed review and for clarifying the Timeline creation behavior. I've addressed the review feedback:
The requested changes are now addressed. Ready for another review. |
PR reviewThe Timeline creation rule is fixed properly: Nothing left in the implementation — everything below is docs. One is merge-blocking and it's a side effect of the fix itself: the Event Timeline page still describes the old creation behavior, the exact bug this round fixed, right under a demo configured to do the opposite. The rest is the two new demos not quite landing visually, and the changelog entry that's still missing after being asked twice. Bugs (0)No findings. Tests (1)1. 🟡 Clearing the resource in single mode is asserted in the UI but never in the payloadLocation: await user.click(await screen.findByRole('option', { name: /no resource/i }));
expect(screen.getByRole('combobox', { name: /resource/i }).textContent).to.match(
/no resource/i,
);The test stops at the combobox text. Failure scenario: A refactor makes the cleared single-select save Fix: Extend that test: save after picking "No resource" and assert Simplifications (1)1. 🟡 The mode is derived and frozen twice, and the two copies have to agreeLocation: const [resourceSelectionMode] = React.useState<ResourceSelectionMode>(() =>
getResourceSelectionMode(
occurrence.resource,
canHaveMultipleResources,
rawPlaceholder?.type === 'creation',
),
);
That's a real invariant resting on an unrelated implementation detail, and the failure is silent: if the tab panel ever became conditionally mounted, switching tabs would re-freeze the section's Failure scenario: Nothing user-visible today; the cost is that two components have to stay in lockstep with no mechanism enforcing it, and the comment in each one explains the freeze without mentioning that the other exists. Fix: Derive it once where Docs (6)1. 🔴 The Event Timeline page still documents the creation bug this commit fixedLocation: A new event created by clicking inside a resource's row starts assigned to that row's resource (a string), so it's edited as single-resource until you turn it into an array yourself. For an event whose `resource` is `null` or not set otherwise (and for any other newly created event), use `canHaveMultipleResources` on `eventCreation` to choose the mode:This paragraph is the old behavior, written back when the row's string resource decided the picker. After the fix, creating in a row seeds So the page shows a demo doing one thing and, two blocks later, tells the reader it can't be done. The second half compounds it: it scopes Failure scenario: A reader evaluating the Event Timeline reads this section, concludes multi-resource events can only be produced by hand-editing the data, and doesn't set the prop. Fix: Replace the paragraph with the rule the code now implements: creating uses 2. 🟡 The shape rule reads as unconditional but only covers editingLocation: - An event whose `resource` is a string is edited as single-resource — the picker shows one entry at a time.
- An event whose `resource` is an array (including `[]`, …) is edited as multi-resource.
Saving never changes that shape: an event that arrives as a string is always saved back as a string …Same block on the timeline page (line 143). "Always" isn't true on the creation path: a Timeline creation placeholder carries a string and, in multiple mode, is saved as an array. On the Event Calendar it happens to hold because creation placeholders never carry a resource, but the sentence is stated as a general rule on both pages, and it's the reason finding 1 reads the way it does. Fix: Say the shape rule applies to existing events, and let the creation paragraph state its own rule. 3. 🟡
|
|
Thanks for the follow-up review. I've addressed the remaining feedback:
Ready for another review. |
PR reviewRound three's feedback is applied almost in full: the payload assertion, the single derivation of the resource selection mode, both docs pages, the JSDoc, and the demo colors all check out against the code and in the browser. The post-merge Nothing is merge-blocking in the code or docs text — the two leftovers below are the Timeline demo and the changelog. Docs (2)1. 🟡 The Timeline demo's titles still truncateLocation: The move to Longer events fix it: with a 3-hour block per event (9:00–12:00 and 13:00–16:00; the first one renamed to something that plausibly lasts that long, since a 3-hour standup doesn't), both titles render in full and the blue/pink duplication is readable in one glance. The 
2. 🟡 The
|
|
Thanks for the feedback. Both changes are now addressed:
Ready for another review. |
rita-codes
left a comment
rita-codes
left a comment
There was a problem hiding this comment.
All feedback addressed — the Timeline demo now renders both titles in full and the changelog entry covers the creation default, canHaveMultipleResources, and the resource: [] clearing behavior.
And with that, this closes the last piece of multi-resource events — the whole feature ships end to end now: rendering, per-row colors, the dialog, creation and editing. 🎉🚀
Huge thanks for driving this over the finish line, @mustafajw07 — a feature this size landing as a community contribution, through four review rounds and a mid-review design change, is genuinely impressive. Congrats! 👏🥳
|
Thank you so much! I really appreciate all the guidance and feedback throughout the process. It was a great learning experience, especially working through the design changes and multiple rounds of review. |
2 participants
Closes #23016
Changelog
New: multi-resource events, end to end
canHaveMultipleResourcesoption oneventCreationcontrols whether new events — and existing events whoseresourceisnull/unset — get a single- or multi-select picker. When not set, it's inferred from youreventsdata: the first event with a definedresourcedecides (a string means single, an array means multiple), and data with no resource at all defaults to multiple.resource: []in multi-resource mode (previouslyundefined/a single id). The Event Timeline still pre-selects the resource of the row you clicked in, but that only seeds an entry —canHaveMultipleResourcesdecides the picker mode either way.shouldEventRequireResourcenow validates that the selection is a non-empty array (at least one resource), instead of just non-null.Behavior change: saving preserves shape
resource: one that arrives as a plain string is always saved back as a string (orundefinedonce cleared), and one that arrives as an array (including[]) is always saved back as an array ([]once cleared). Only a new event, or an existing one with no resource shape to begin with, is subject tocanHaveMultipleResources.resource: []rather than leaving itundefined.Fixed
Event Timeline: a multi-resource event now renders with each row's own
eventColorinstead of always taking its primary resource's color in every row. An event's owncolorproperty still wins everywhere, in every row.Event dialog: an event referencing a resource id no longer present in
resources(e.g. a deleted resource) no longer shows the dashed "no resource" swatch — that state is now correctly distinguished from having nothing selected.I have followed (at least) the PR section of the contributing guide.