-
Notifications
You must be signed in to change notification settings - Fork 887
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
just a note: you can use the @coder/core-ts group to avoid getting kira every time 😄 |
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 it InboxPopover
and NotificationsInbox
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 name NotificationsInbox
and NotificationsInboxView
.
site/src/modules/notifications/NotificationsInbox/InboxPopover.stories.tsx
Show resolved
Hide resolved
site/src/modules/notifications/NotificationsInbox/InboxPopover.tsx
Outdated
Show resolved
Hide resolved
site/src/modules/notifications/NotificationsInbox/NotificationsInbox.stories.tsx
Outdated
Show resolved
Hide resolved
site/src/modules/notifications/NotificationsInbox/NotificationsInbox.stories.tsx
Outdated
Show resolved
Hide resolved
site/src/modules/notifications/NotificationsInbox/NotificationsInbox.tsx
Outdated
Show resolved
Hide resolved
site/src/modules/notifications/NotificationsInbox/NotificationsInbox.stories.tsx
Outdated
Show resolved
Hide resolved
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! |
Related to coder/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 this notion doc.
What is not included
How to test the components?