- Notifications
You must be signed in to change notification settings - Fork1k
chore: add notification UI components#16818
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
aslilac commentedMar 5, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
just a note: you can use the @coder/core-ts group to avoid getting kira every time 😄 |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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.
it seems like a lot of the stories itInboxPopover
andNotificationsInbox
are redundant. I'd rather just pick which level we want to test it at and not duplicate all of them.
also, the fact that the names are so different but the stories/appearance are so similar confused me for a bit. this seems like the sort of thing we would usually nameNotificationsInbox
andNotificationsInboxView
.
site/src/modules/notifications/NotificationsInbox/InboxPopover.stories.tsxShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
site/src/modules/notifications/NotificationsInbox/InboxPopover.tsx OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
site/src/modules/notifications/NotificationsInbox/NotificationsInbox.stories.tsx OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
site/src/modules/notifications/NotificationsInbox/NotificationsInbox.stories.tsx OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
site/src/modules/notifications/NotificationsInbox/NotificationsInbox.tsx OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
site/src/modules/notifications/NotificationsInbox/NotificationsInbox.stories.tsx OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
I think they are testing different things, but I agree they look similar because they are performing similar actions, but with a different context. In the view, I'm testing if things are displayed correctly given the props. In the "container", I want to test the component as whole when dealing with async calls and their errors. Makes sense?
Hm... I think it makes sense. cc.:@aslilac |
@aslilac it makes sense to me, so we could say when having the container/view pattern, when testing the container, we are already testing the view making tests in the view layer redundant/unnecessary. Sounds correct? 🤔 I'm phrasing this because it would be nice to have it written somewhere like in the tests section in the FE contributing guide. |
And I'm wondering, if that is the case, what is the benefit of splitting the container and view components? 🤔 |
in my mind, it's been so that we could test the views in storybook and the containers in e2e tests. I don't think storybook/jest are really very good for testing container components, because all it's really testing is "did you set up a bunch of mocks the way the component expects" and not "does the component work properly when connected to the backend".sometimes those are the same thing, but if your component misunderstands what it should be getting from the backend in some way, they are very very different, and jest/storybook cannot catch those kinds of issues. maybe we should talk about this at the next variety meeting and codify it somewhere if everyone agrees that that sounds reasonable. |
It sounds very reasonable to me 🤔. Should we start to require e2e tests when working on page components? At least the ones that have some server interaction. |
btw, since we're planning on having some larger conversations about testing, I won't block approval on the testing concerns. just waiting for some of the other things to be addressed. |
@aslilac done! |
f2cd046
intomainUh oh!
There was an error while loading.Please reload this page.
Related tocoder/internal#336
This PR adds the base components for the Notifications UI below (you can click on the image to open the related Figma design) based on the response structure defined on thisnotion doc.
What is not included
How to test the components?