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

docs(blade): Toast API decisions #1990

Merged
merged 16 commits into from
Feb 14, 2024
Merged

docs(blade): Toast API decisions #1990

merged 16 commits into from
Feb 14, 2024

Conversation

anuraghazra
Copy link
Member

@anuraghazra anuraghazra commented Jan 23, 2024

Copy link

changeset-bot bot commented Jan 23, 2024

⚠️ No Changeset found

Latest commit: e24adc6

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

github-actions bot commented Jan 23, 2024

✅ PR title follows Conventional Commits specification.

Copy link

codesandbox-ci bot commented Jan 23, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit e24adc6:

Sandbox Source
razorpay/blade: basic Configuration

@anuraghazra anuraghazra added the Review - L1 First level of review label Jan 24, 2024
/**
* Can be used to render an icon
*/
leading?: IconComponent;
Copy link
Collaborator

Choose a reason for hiding this comment

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

since we don't have trailing counterpart and we know that icon will always be leading wouldn't it be better to name it just icon? so it will also be consistent with rest of the components?

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1. Lets just call it icon similar to how we do it for button

Copy link
Member Author

Choose a reason for hiding this comment

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

In promotional toast the leading can also become an asset, so leading maybe more flexible naming here.

image

Copy link
Collaborator

Choose a reason for hiding this comment

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

What do we mean by asset? It can be an image or text or video? Any restrictions or is it a slot?

Copy link
Member Author

Choose a reason for hiding this comment

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

No restrictions, it's a slot. we should atleast keep the prop name generic to cater to future usecase.

Same thing we did for BottomSheetHeader too.

Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe we can have icon and asset as a naming then?

Copy link
Member Author

Choose a reason for hiding this comment

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

Two props? Will cause impossible states.


```ts
type useToastReturnType = {
/**
Copy link
Collaborator

Choose a reason for hiding this comment

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

we shall also give an option to know the current id of the toast that is opened?

Copy link
Member Author

Choose a reason for hiding this comment

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

"current" can also be multiple toasts, we can give an option like:

const { toasts } = useToasts();

but not sure about the usecase.


const Example = () => {
const toastId = React.useRef(null);
const { showToast, dismissToast } = useToast();
Copy link
Collaborator

Choose a reason for hiding this comment

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

toast will have a context at the app level right? can't see that in the example?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe, I'm not sure right now actually, so react-hot-toast works in a way that it doesn't need ReactContext.
In the current POC that I've done that also didn't need a toast context.

const App = () => {
return (
<BladeProvider>
<ToastContainer position="bottom-left" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will there be multiple toast containers in an app?

Can there not be a single ToastProvider that is part of BladeProvider and useToast can decide the toast position?

Copy link
Member Author

Choose a reason for hiding this comment

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

There can only be 1.

Can there not be a single ToastProvider that is part of BladeProvider and useToast can decide the toast position?

That should be possible, we can keep the ToastContainer inside BladeProvider but the thing is we also may need to pass some props to ToastContainer, how should we handle that?

@anuraghazra anuraghazra marked this pull request as ready for review January 31, 2024 07:16
/**
* Called when the toast is dismissed or duration runs out
*/
onDismissButtonClick?: () => void;
Copy link
Member

Choose a reason for hiding this comment

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

perhaps single onDismiss with type = 'auto' | 'onclick'?

/**
* Called when the toast is dismissed or duration runs out
*/
onDismissButtonClick?: () => void;
Copy link
Member

Choose a reason for hiding this comment

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

onAutoDismiss & onManualDismiss

- Q. What should be the default duration for auto dismissable toasts?
- A. 4s for informational toasts and 6s for promotional toasts

- Q. Should we call it `onDismissButtonClick` or `onDismiss`?
Copy link
Member

Choose a reason for hiding this comment

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

onManualDismiss

Copy link
Member Author

Choose a reason for hiding this comment

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

Going ahead with onDismissButtonClick, let's not have onManual, onAutoDismiss because there is no controlled state that users can manage for toasts so they don't really need to know if a toast is auto dismissed or not.

@saurabhdaware
Copy link
Member

Can you check these 2 comments? they are related to API decision

@anuraghazra
Copy link
Member Author

Can you check these 2 comments? they are related to API decision

Responded.


### Promotional Toast

<img src="./example-promotional-toast.png" width="380" alt="Promotional toast example" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

add anatomy wala screenshot here? primarily because icon is asset in promotional

Copy link
Member Author

@anuraghazra anuraghazra Feb 12, 2024

Choose a reason for hiding this comment

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

add anatomy wala screenshot here

Design hasn't created anatomy illustration for promo toast. // @saurav12

primarily because icon is asset in promotional

There is no difference of anatomy between info/promo toast. Both are 1:1

  • Leading: icon or assset
  • Content: JSX element (to put bold/italic text inside info)
  • Action Button
  • Dismiss Button

/**
* @default `neutral`
*/
color?: 'neutral' | 'positive' | 'negative' | 'warning' | 'information'
Copy link
Collaborator

Choose a reason for hiding this comment

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

in case of promotional we omit this prop?

Copy link
Member Author

Choose a reason for hiding this comment

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

We just ignore this prop in case of promo.

* @default true - for informational toast
* @default false - for promotional toast
*/
autoDismiss?: boolean;
Copy link
Collaborator

Choose a reason for hiding this comment

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

is there a use case for promotional toast to be auto dismissed?

Copy link
Member Author

Choose a reason for hiding this comment

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

That would be a product decision. We do not have a defined usecase from design side.

/**
* @returns id of the toast
*/
show: (toast: Toast) => string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

this will return id of which toast since there could be many?

Copy link
Member Author

Choose a reason for hiding this comment

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

Id of the toast just being created via this show function.

- Q. Should we call it `onDismissButtonClick` or `onDismiss`?

- Q. Should the dismiss handler be called even when the toast is auto dismissed? Or should we have different handlers for auto dismiss and manual dismiss? (eg: `onAutoDismiss` `onDismissButtonClick`)
- A. We will call it `onDismissButtonClick` and it will only be called when the user clicks on the dismiss button.
Copy link
Collaborator

Choose a reason for hiding this comment

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

if product needs to track the event on auto dismiss then how can consumer handle that??

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if tracking auto dismiss makes sense.

if 2 toasts are opened with autoDismiss: true
then 2 toasts will be dismissed after 4s for sure.

They could just track if 2 toasts are opened or not.

onClick={() => {
  toast.show();
  analytics.track();
}}

Copy link
Collaborator

Choose a reason for hiding this comment

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

you can do multiple things like for eg: a user was shown a toast and they navigate to a different page then when the toast is dismissed you can track the URL of the page. this way you can identify a pattern that after performing some actions what are the pages a user navigates to and get some insights around user behavior

Copy link
Member Author

Choose a reason for hiding this comment

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

a user was shown a toast and they navigate to a different page then when the toast is dismissed you can track the URL of the page

That is possible.

onDismissButtonClick={() => naigateToPage()}

But don't think it will be a very good idea for toast to automatically navigate the page after dismiss. Navigation should happen after action.

Copy link
Member Author

@anuraghazra anuraghazra Feb 14, 2024

Choose a reason for hiding this comment

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

FYI,

react-hot-toast doesn't have a callback that it calls when a toast gets auto dismissed, so it's not even possible to have an onAutoDismiss method.

There is a workaround but doing that means you need to keep track of each and every toast's timer manually, too messy.

@kamleshchandnani kamleshchandnani merged commit 8f3d52f into master Feb 14, 2024
14 checks passed
@kamleshchandnani kamleshchandnani deleted the anu/toast-api branch February 14, 2024 08:49
anuraghazra added a commit that referenced this pull request Apr 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review - L1 First level of review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants