-
Notifications
You must be signed in to change notification settings - Fork 2
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 reusable ToastMessages component #1272
Conversation
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.
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.
Brought verbatim from client https://github.com/hypothesis/client/blob/2fa444e05d806893606a9181423d9d4f35bfbc7f/src/shared/components/test/ToastMessages-test.js
I only added a small fix to make sure there are no components left in the DOM that have side effects in other tests.
96bca8f
to
cc26572
Compare
Codecov Report
@@ Coverage Diff @@
## main #1272 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 61 62 +1
Lines 844 895 +51
Branches 332 344 +12
=========================================
+ Hits 844 895 +51
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
cc26572
to
f410dfe
Compare
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.
LGTM with some minor comments. I would leave the ones about changing the arguments to the onDismiss
callback for a subsequent revision.
src/pattern-library/components/patterns/feedback/ToastMessagesPage.tsx
Outdated
Show resolved
Hide resolved
src/pattern-library/components/patterns/feedback/ToastMessagesPage.tsx
Outdated
Show resolved
Hide resolved
|
||
export type ToastMessagesProps = { | ||
messages: ToastMessage[]; | ||
onMessageDismiss: (id: string) => void; |
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.
The fact that we pass a list of ToastMessage
s as input, but only emit the id
in onMessageDismiss
feels a little incoherent and is mainly a relic of use in the client. It would probably make more sense to emit the ToastMessage
or index itself. Probably best to get the initial shared iteration landed first though.
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.
I'll create a follow-up issue to change this for next major version
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.
src/pattern-library/components/patterns/feedback/ToastMessagesPage.tsx
Outdated
Show resolved
Hide resolved
src/pattern-library/components/patterns/feedback/ToastMessagesPage.tsx
Outdated
Show resolved
Hide resolved
8cd2e19
to
7b6624f
Compare
7b6624f
to
cd532e3
Compare
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.
I had a few minor corrections and comments, otherwise looks good.
src/pattern-library/components/patterns/feedback/ToastMessagesPage.tsx
Outdated
Show resolved
Hide resolved
cd532e3
to
ca64c11
Compare
Closes #1075
This PR brings the
ToastMessages
component from client/via, and adds the corresponding documentation to the pattern library.Once this is merged, we can start using it in frontend apps and remove duplicated code.
Testing steps
make dev
)