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

add domProps prop to toast #6200

Merged
merged 6 commits into from
Apr 25, 2024
Merged

add domProps prop to toast #6200

merged 6 commits into from
Apr 25, 2024

Conversation

snow893
Copy link
Contributor

@snow893 snow893 commented Apr 12, 2024

Closes #6206

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

@davidcoleman007
Copy link

Thank you @snow893 for adding this. It will be very helpful for our automation.

@snow893 snow893 force-pushed the snow/toastTestId branch 2 times, most recently from 11a41f0 to 2d06bb2 Compare April 15, 2024 21:55
@snowystinger
Copy link
Member

Thanks for the test!

I think I prefer adding DOM props and the filterDOMProps to match our other components.

The only hangup I have is that in the storybook example and tests, any and all toasts created would have the same testid. If we did allow DOM props, we'd be allowing id, which must be unique. I don't know if testid tries to follow this ideal as well, but it's pretty clear this could be an easy set of props to misuse.

@davidcoleman007
Copy link

Thanks for the test!

I think I prefer adding DOM props and the filterDOMProps to match our other components.

I think i prefer this too. Some conventions use data-testid and others use data-test-id. it should be the consumer's choice which they use.

The only hangup I have is that in the storybook example and tests, any and all toasts created would have the same testid.

this speaks to implementation of id passed by consumer

If we did allow DOM props, we'd be allowing id, which must be unique. I don't know if testid tries to follow this ideal as well
Test id's identify a component, uniquely or abstractly.

A test id should be "unique" in the context which it exists, but not necessarily across the app. Some components may take identify a container, with out any additional context and maybe there may be identifying children of this container. automation frameworks also support finding "lists" of elements for a single selector, so while they may not be the usual case, it is supported. Uniqueness, or the lack thereof - is the responsibility of the consumer who supplies the id.

however let's stick with toast - In this case, the user should be supplying something unique to the toast, which causes us to be able to either uniquely identify each toast, If we supply the same children for a different purpose, we can provide a different test-id.

it is also realistic to presume that when a client creates a toast, there will be some api where i can specify the test id of my toast, and it is now my responsibility to name my toast unique to identify myself without any compound expression. ie: 'file-too-big-toast'. This means that to your example of "all toasts in the story book would have the same id", I would say "each use case should toast their own unique id." Passing through the value given from the consumer doesn't protect us from id collisions or conflicts, but it is consistent with how a consumer expects this to work. two invocations of 'file-too-big-toast' should mean the same thing, and there's no reason that we will want to distinguish this. if a client wishes to pass 'file-too-big-toast-${key}' they can solve the problem of multiple toasts being uniquely identifiable.

@snowystinger
Copy link
Member

Agreed, my only reason for commenting it was that this is a non-traditional API (ie not JSX) for creating a component. And as we see in the story, it's easy to accidentally make duplicates.

Yes, you can make duplicate ids in other situations, but generally you're thinking about it in terms of the HTML attribute, not some options object.

Not a blocker, just something to think about if we have any ideas to make it easier from this side.

@snow893 snow893 changed the title add testid prop to toast add domProps prop to toast Apr 17, 2024
@snow893
Copy link
Contributor Author

snow893 commented Apr 17, 2024

@snowystinger updated to domProps

@snow893 snow893 force-pushed the snow/toastTestId branch 2 times, most recently from 359bb4f to 1fb6d08 Compare April 18, 2024 00:00
i saw nothing

test

allow more toast props

review

props

-_-
snowystinger
snowystinger previously approved these changes Apr 18, 2024
@snow893
Copy link
Contributor Author

snow893 commented Apr 23, 2024

@reidbarber @devongovett do you have any objections / change requests?

@yihuiliao yihuiliao merged commit 1550cca into adobe:main Apr 25, 2024
25 checks passed
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.

Toasts don't support testid (or other data props)
6 participants