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

feat(Toaster): add "has" method #439

Merged
merged 5 commits into from
Mar 6, 2023
Merged

Conversation

oynikishin
Copy link
Contributor

No description provided.

@gravity-ui-bot
Copy link
Contributor

gravity-ui-bot commented Dec 22, 2022

Preview is ready.

src/components/Toaster/Toast/Toast.tsx Outdated Show resolved Hide resolved
src/components/Toaster/types.ts Outdated Show resolved Hide resolved
@amje amje changed the title feat(Toaster): add lifecircle callbacks feat(Toaster): add lifecycle callbacks Dec 27, 2022
@oynikishin oynikishin force-pushed the feat/toaster/lifecircle-callbacks branch 2 times, most recently from dc6a46b to e1c6a68 Compare December 28, 2022 18:58
@oynikishin oynikishin force-pushed the feat/toaster/lifecircle-callbacks branch from e1c6a68 to b378f6b Compare January 11, 2023 13:02
| type | `string` | | `undefined` | Notification type. Possible values: `error`, `success`. If `type` is set, icon (success/error) will be added into notification title. _By default there is no icon_ |
| isClosable | `boolean` | | `true` | Configuration that manages visibility of cross icon, which allows to close notification |
| actions | `ToastAction[]` | | `undefined` | Array of [actions](./types.ts#L9) which displays after `content` |
| onMount | `ToastLifecycleCallback` | | `undefined` | Callback. Fired when corresponding toast component mount |
Copy link
Contributor

Choose a reason for hiding this comment

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

I think callback word is redundant

onUnmount({props, element: heightProps.ref.current});
}
};
}, []);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why there are no onMount, onUnmount dependencies here?

Copy link
Contributor Author

@oynikishin oynikishin Jan 11, 2023

Choose a reason for hiding this comment

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

I don't want to do smth when onMount or onUnmount changes.
onMount and onUnmount should fired only once during corresponding stages.
Using React.useEffect without deps is common pattern for this case as I know.
Actual value of these functions will be used when time comes.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is a little bit unfair. Component does not detect changing of this props. Maybe you should add ref that stores value isMount and use it in effect

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. I'll leave it up to Alexey @ogonkov :)

Copy link
Contributor

@ValeraS ValeraS left a comment

Choose a reason for hiding this comment

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

What task do we solve here? We need callbacks for toast addition and deletion events, right? If so, it is better to call them from appropriate methods from ToasterProvider (add, remove, removeAll).

@oynikishin
Copy link
Contributor Author

The main goal is to provide API that helps developers to understand when they cannot reuse (update) old toast and have to create another one.

I chose to provide unmount event because it is easy to understand API and solves my problem not ideal but well enough.
(mount is added for symmetry).

The problem with remove is that I don't quite understand what it is.
Technically a toast is removed when it is deleted from the list of toasts.
But in fact a toast becomes "invalid" when it enters the ToastStatus.Hiding stage.
We can still change the content but we can no longer affect allowAutoHiding and timeout props.
And maybe it would be better for the developer to recreate the toast and not try to update the current hiding one.

Maybe it will be better to provide a ToastStatus change event with extra status like removed..?
I need your advice!

@ogonkov
Copy link
Contributor

ogonkov commented Feb 9, 2023

Can you provide example of usage of this api?

@oynikishin
Copy link
Contributor Author

oynikishin commented Feb 10, 2023

Can you provide example of usage of this api?

I need smth like this:

    const createUpdatableToast = (name: string, title: string) => {
        let toastExists = false;
        const addToast = (newTitle: string) => {
            toaster.add({
                name,
                title: newTitle,
                onUnmount: () => {
                    toastExists = false;
                }
            });
            toastExists = true;
        };

        const addOrUpdateToast = (newTitle: string) => {
            if (toastExists) {
                toaster.update(name, { title: newTitle});
            } else {
                addToast(newTitle);
            }
        };

        addToast(title);
        return addOrUpdateToast;
    }

There is several ways to solve my problem:

  • different variants of close callback (at least three))
  • or maybe some method like addOrUpdate (...why not)

As I already mentioned.
I chose mount/unmount event because it is enough for my case and it is well self-documented api.

But may be you suggest the best way!

@ogonkov ogonkov self-assigned this Feb 20, 2023
@oynikishin oynikishin force-pushed the feat/toaster/lifecircle-callbacks branch 2 times, most recently from eb3cf89 to 70e73ca Compare March 2, 2023 14:54
@oynikishin oynikishin changed the title feat(Toaster): add lifecycle callbacks feat(Toaster): add "has" method Mar 2, 2023
describe('api.has', () => {
it('should return false when toast is not added', () => {
const providerAPI = setup();
expect(providerAPI.has('unexisted toasts')).toBeFalsy();
Copy link
Contributor

Choose a reason for hiding this comment

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

toBe(false)

you interested in returning boolean from api, not some falsy value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

@oynikishin oynikishin requested a review from ValeraS March 2, 2023 15:55
@oynikishin oynikishin force-pushed the feat/toaster/lifecircle-callbacks branch from c0cfcc9 to 4061b95 Compare March 2, 2023 18:00
@@ -60,20 +60,28 @@ export const ToasterProvider = React.forwardRef<ToasterPublicMethods, Props>(
[],
);

const toastsRef = React.useRef<InternalToastProps[]>();
toastsRef.current = toasts;
Copy link
Contributor

Choose a reason for hiding this comment

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

React documentation recommends not writing or reading ref.current during rendering https://beta.reactjs.org/reference/react/useRef#caveats

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@oynikishin oynikishin force-pushed the feat/toaster/lifecircle-callbacks branch from 3f3335a to a5ee38a Compare March 6, 2023 14:28
@oynikishin oynikishin merged commit ea2b2d8 into main Mar 6, 2023
@oynikishin oynikishin deleted the feat/toaster/lifecircle-callbacks branch March 6, 2023 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants