Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
BrunoQuaresma merged 6 commits intomainfrombq/notification-components
Mar 12, 2025

Conversation

BrunoQuaresma
Copy link
Collaborator

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.

new notifications including hover

What is not included

  • Support for infinite scrolling (pending on BE definition)

How to test the components?

  • The only way to test the components is to use Chromatic or downloading the branch and running Storybook locally.

@BrunoQuaresmaBrunoQuaresma self-assigned thisMar 5, 2025
@BrunoQuaresmaBrunoQuaresma requested review fromKira-Pilot,a team andaslilac and removed request fora team andKira-PilotMarch 5, 2025 20:19
@aslilac
Copy link
Member

aslilac commentedMar 5, 2025
edited
Loading

just a note: you can use the @coder/core-ts group to avoid getting kira every time 😄

BrunoQuaresma reacted with thumbs up emojiKira-Pilot reacted with laugh emoji

Copy link
Member

@aslilacaslilac left a 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.

@BrunoQuaresma
Copy link
CollaboratorAuthor

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.

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?

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.

Hm... I think it makes sense.

cc.:@aslilac

@aslilac
Copy link
Member

I think they are testing different things

Screenshot 2025-03-07 at 3 03 07 PM

The tests feel a bit like this to me, which is why I think they're redundant. Everything being tested in one is fully tested in the other from my point of view.

@BrunoQuaresma
Copy link
CollaboratorAuthor

The tests feel a bit like this to me, which is why I think they're redundant. Everything being tested in one is fully tested in the other from my point of view.

@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.

@BrunoQuaresma
Copy link
CollaboratorAuthor

And I'm wondering, if that is the case, what is the benefit of splitting the container and view components? 🤔

@aslilac
Copy link
Member

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.

BrunoQuaresma reacted with thumbs up emoji

@BrunoQuaresma
Copy link
CollaboratorAuthor

in my mind, it's been so that we could test the views in storybook and the containers in e2e tests.

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.

@aslilac
Copy link
Member

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.

BrunoQuaresma reacted with thumbs up emoji

@BrunoQuaresma
Copy link
CollaboratorAuthor

@aslilac done!

@BrunoQuaresmaBrunoQuaresma merged commitf2cd046 intomainMar 12, 2025
30 checks passed
@BrunoQuaresmaBrunoQuaresma deleted the bq/notification-components branchMarch 12, 2025 17:36
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsMar 12, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@jaaydenhjaaydenhjaaydenh left review comments

@aslilacaslilacaslilac approved these changes

@defelmnqdefelmnqAwaiting requested review from defelmnq

@DanielleMaywoodDanielleMaywoodAwaiting requested review from DanielleMaywood

Assignees

@BrunoQuaresmaBrunoQuaresma

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

3 participants
@BrunoQuaresma@aslilac@jaaydenh

[8]ページ先頭

©2009-2025 Movatter.jp