chore(structure): remove TimelineStore from <DocumentPaneProvider/>
#8271
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Skipped Deployments
|
|
No changes to documentation |
|
Component Testing Report Updated Jan 20, 2025 10:52 AM (UTC) ❌ Failed Tests (2) -- expand for details
|
⚡️ Editor Performance ReportUpdated Mon, 20 Jan 2025 10:53:17 GMT
Detailed information🏠 Reference resultThe performance result of
🧪 Experiment resultThe performance result of this branch
📚 Glossary
|
There was a problem hiding this comment.
Great work! I just a couple of questions. :)
Also, I just want to get one more pair of eyes on this beyond my own since I do feel that we might need to add something extra in the changelog about potential breaking changes? But I might be exaggerating that aspect, just want another opinion 🙏
I tagged you @bjoerge but let me know if you are swamped and I'll round robin someone else into the conversation 👍
| @@ -9,9 +9,14 @@ import {type TimelineState, type TimelineStore} from './useTimelineStore' | |||
| * @internal | |||
| */ | |||
| export function useTimelineSelector<ReturnValue>( | |||
| timelineStore: TimelineStore, | |||
| timelineStore: TimelineStore | undefined, | |||
There was a problem hiding this comment.
A tad concerned about this change, this will be a breaking change since users so far haven't had to be concerned about potentially sending an undefined through here.
Though the more I think about it might be fine? Since overall the users might need to make some extra changes? We do still keep the legacy components 🤔
There was a problem hiding this comment.
Very valid concern, I'm not sure how we should handle this type of changes. However I highly doubt that any customer is using this, and if they are using it they are probably depending on the useTimelineSelector which is updated to match this new possibly undefined type.
This won't be undefined yet, it will be once we merge corel and we enable the eventsAPI.
We also have the option here to not expose this as undefined just yet and do that change as part of corel
packages/sanity/src/structure/panes/document/DocumentPaneProviderWrapper.tsx
Show resolved
Hide resolved
| setTimelineMode?: undefined | ||
| /** | ||
| * @deprecated not used anymore | ||
| * */ | ||
| timelineMode?: undefined |
There was a problem hiding this comment.
I don't know what's our current go to approach for deprecating properties. I think the way we've been doing it is first putting the @deprecated label, leaving it to simmer for a bit as a warning for folks not to use it and only later removing it instead of changing the type now since this will cause an immediate breaking change
I'm not sure who to tag so let's say @bjoerge , do you have thoughts?
There was a problem hiding this comment.
It has been unused by us since we merged the History UI updates and it hasn't made much sense since then.
Would like to understand how to properly handle this deprecations.
This was previously used to determine which Timeline Selection menu was open, when we had the option to select the document from the document header, with the new this is not needed.
There was a problem hiding this comment.
huh, if this is the case I would almost prefer to remove it (ripping off the bandaid) which is certainly more extreme but I feel it would be preferrable vs a middle ground like this. Though maybe the approach should be as I mentioned above, I'm not sure
We should probably overall clarify how do we want to handle these cases.
There was a problem hiding this comment.
Seems unlikely that anyone would rely on this? If it's no longer in use, it should be fine to remove it.
There was a problem hiding this comment.
Yeah, I have the same feeling, and if they are depending on it they should update it because it's not respected by us anymore.
Thanks for reviewing @RitaDias , just updated the notes for release to reflect this two deprecations. |
pedrobonamin
force-pushed
the
corel-322
branch
from
4624556 to
53d3bd2
Compare
pedrobonamin
force-pushed
the
corel-322
branch
from
53d3bd2 to
247c066
Compare
Description
We have always had only one way to represent the Timeline, it has been defined by the TimelineStore, with the addition of releases we are adding a new store that builds on the events api.
This PR updates the
DocumentPaneProviderto receive the timeline through the props, allowing us to inject either this Legacy Timeline or the new Events API.It also changes the
structureexport to expose theDocumentPaneProviderWrapperinstead of theDocumentPaneProviderThe wrapper now only inserts the legacy Timeline, but in
corelit will check the feature flag and use the events store instead.This PR also removes the
timelineModeproperty from the document pane, given it's not used anywhere. This is a legacy property that was used before the History UI updatesWhat to review
Is it ok to rename the export as done in here?
Is it ok to deprecate the TimelineMode property as this PR is doing it?
Testing
Existing tests should cover the functionalities, nothing new is added. This PR only moves things around to make it possible to inject a different timeline provider
Notes for release
fix(core): remove timelineMode from
documentPaneContextand makesTimelineStoreoptional.Deprecations:
useDocumentPane().timelineModeproperty is now deprecated, this hasn't been used since the updates to the History UI.useDocumentPane().timelineStorepossibly undefined, preparing for the introduction of the events API, this property could be undefined once[corel](https://github.com/sanity-io/sanity/tree/corel)is merged.