Skip to content
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

refactor(Toaster): fix incompatibility of different Toaster APIs #1987

Open
wants to merge 2 commits into
base: next
Choose a base branch
from

Conversation

icekimi23
Copy link
Contributor

@icekimi23 icekimi23 commented Dec 11, 2024

PR is made to implement this RFC gravity-ui/rfc#15

Breaking changes:

  • ToasterProvider now requires a mandatory toaster property, where you must pass an instance of the Toaster class.

  • The Toaster class is now responsible for managing toast state but no longer handles rendering. As a result, there is no need to call Toaster.injectReactDOMClient(ReactDOMClient) anymore.

  • Calling new Toaster() now returns a new object each time. If you want to use toasts within React through the useToaster() hook or use the Toaster object directly outside React (e.g., in Redux side effects, etc.) while ensuring that toasts appear in a same screen area without overlapping, you need to make sure that the object passed to ToasterProvider and the one used outside React is the same instance.

  • To get a single shared instance of Toaster, you can import it from @gravity-ui/uikit/toaster-singleton.

  • The toaster-singleton-react-18.js file no longer exists, but it is unnecessary since there is no longer any specific dependency on a particular React version.

@gravity-ui-bot
Copy link
Contributor

Preview is ready.

@gravity-ui-bot
Copy link
Contributor

Visual Tests Report is ready.

remove(name: string) {
this.toasts = removeToast(this.toasts, name);

this.notify();
}
Copy link
Contributor

@ogonkov ogonkov Dec 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to maintain destroy() method, let it call removeAll underneath


import {ToasterContext} from './ToasterContext';
import {ToastsContext} from './ToastsContext';

type Props = React.PropsWithChildren<{}>;
type Props = React.PropsWithChildren<{
toaster: ToasterSingleton;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is definitely breaking change, please change target branch to next

Comment on lines 94 to 98
private notify() {
for (const listener of this.listeners) {
listener(this.toasts);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please separate subscribe pattern from Toaster class to separate base class EventEmitter

@ogonkov
Copy link
Contributor

ogonkov commented Dec 17, 2024

Please describe all breaking changes in PR description

@icekimi23
Copy link
Contributor Author

@ogonkov I have updated PR, check it once again, please, when you can. I removed Singleton pattern from Toaster (in separate commit), I think it is more clear and flexible to return new object every time, with this behaviour users can create multiple Providers for toasts if they want. But it would be nice to hear your opinion:)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants