-
-
Notifications
You must be signed in to change notification settings - Fork 353
[toasts] Introduce a store #3464
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
commit: |
Bundle size report
Check out the code infra dashboard for more information about this PR. |
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
e036c1b to
529e85e
Compare
97a71e8 to
728c2c3
Compare
|
|
||
| export interface ToastManagerUpdateOptions<Data extends object> | ||
| extends Partial<ToastManagerAddOptions<Data>> {} | ||
| extends Partial<Omit<ToastObject<Data>, 'id'>> {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This allows to use store.updateToast instead of store.set('toasts, ....) in Toast.Root.
If you want to keep some properties private (even though their is no runtime safe guard, it's only typing), I can create a store.updateToastPublic method that keeps the previous typing and expose it in Toast.useToastManager instead of store.updateToast
| return ( | ||
| <ToastViewportContext.Provider value={contextValue}> | ||
| {numToasts > 0 && prevFocusElement && <FocusGuard onFocus={handleFocusGuard} />} | ||
| <React.Fragment> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This context was never used.
But if we need viewportRef, we can now use useToastContext since it's stable accross render 👌
| resumeTimers, | ||
| viewportRef, | ||
| windowFocusedRef, | ||
| setFocused, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me the logic still works here but I would like a double check 👀
|
I'd rather not include this in v1. Introducing a Store to other components usually came with a couple of subtle bugs, so let's test it without rushing after the release. |
Clearly, I'm not in favor of rushing anything before the stable, unless the gains are huge 👍 |
Greptile OverviewGreptile SummaryThis PR successfully refactors the toast system to use a centralized store pattern, significantly improving code organization and performance. Key Changes:
Benefits:
Confidence Score: 5/5
Important Files Changed
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
6 files reviewed, 6 comments
packages/react/src/utils/temporal/field/TemporalFieldSectionListPropsPlugin.ts
Outdated
Show resolved
Hide resolved
packages/react/src/utils/temporal/field/TemporalFieldSectionListPropsPlugin.ts
Outdated
Show resolved
Hide resolved
packages/react/src/utils/temporal/field/TemporalFieldSectionListPropsPlugin.ts
Outdated
Show resolved
Hide resolved
packages/react/src/utils/temporal/field/TemporalFieldSectionListPropsPlugin.ts
Outdated
Show resolved
Hide resolved
|
@michaldudak this PR is ready for another round of review 🙏 |
|
Review by Codex: The new |
|
@michaldudak I think the did the review before this morning's fix. Analysis updateToast(id: string, updates: ToastManagerUpdateOptions) { The Fix (Already Applied) Cancels timers when timeout: 0 is set or type changes to loading Verdict timeout: 0 → positive value: starts auto-dismiss timer |
|
Yup, Codex agrees, it's fixed :) |
| export const ToastContext = React.createContext<ToastContext<any> | undefined>(undefined); | ||
| export const ToastContext = React.createContext<ToastContext | undefined>(undefined); | ||
|
|
||
| export function useToastContext() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we can call it useToastStore to differentiate it from useToastRootContext?
michaldudak
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one nit. Besides that it looks good to me
This should not be part of the stable release it can be reviewed after 👍
ToastProvideruseStableCallback