Skip to content

Web: Add notification store#32970

Merged
kimlisa merged 9 commits intomasterfrom
lisa/web/notification-bell
Oct 12, 2023
Merged

Web: Add notification store#32970
kimlisa merged 9 commits intomasterfrom
lisa/web/notification-bell

Conversation

@kimlisa
Copy link
Copy Markdown
Contributor

@kimlisa kimlisa commented Oct 4, 2023 •
edited
Loading

part of https://github.com/gravitational/teleport.e/issues/2242

note: recommend reviewing by commit

  • Adds a notification store
    • only supports access_list notifications atm (for review due dates), and first iteration, it will be dumb in that we just fetch it upon init, and updates whenever a user views, lists, edits access list (this is in another PR)
    • not sure how this will grow in the future because we only have one use case, so i tried to make it kinda generic
  • Adds a notification bell on the topbar, when there are notification a attention dot renders
  • Adds a attention dot to main navigation switcher and on the navigation item

with the brain:

image

Can view demos in storybook:

image image image image image image

@kimlisa kimlisa force-pushed the lisa/web/notification-bell branch from 4ccb852 to 0b80215 Compare October 4, 2023 16:33
@kimlisa kimlisa marked this pull request as ready for review October 4, 2023 16:33
@github-actions github-actions Bot requested review from gzdunek and ryanclark October 4, 2023 16:34
kimlisa
kimlisa commented Oct 4, 2023
View reviewed changes
Comment thread web/packages/teleport/src/Navigation/Navigation.tsx
Comment on lines +179 to +184
items={[
{
category: NavigationCategory.Management,
requiresAttention: ctx.storeNotifications
.getNotifications()
.some(n => n.kind === 'access-lists'),
},
{ category: NavigationCategory.Resources },
]}
Copy link
Copy Markdown
Contributor Author

@kimlisa kimlisa Oct 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

woops... this should've been in the previous commit

@kimlisa kimlisa force-pushed the lisa/web/notification-bell branch 2 times, most recently from 8915843 to 5238f5d Compare October 6, 2023 00:52
@kimlisa
Copy link
Copy Markdown
Contributor Author

kimlisa commented Oct 6, 2023

friendly ping @gzdunek @ryanclark

gzdunek
gzdunek reviewed Oct 6, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@gzdunek gzdunek left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI we already have Notifications in shared (you can find them in storybook). We are using them in Connect and Isaiah used them for a desktop session warnings. I'm wondering if these things could (and should) be merged.
I also know that our design team is working on a design system for notifications.

Probably these things are different enough that they should be separated (notifications from shared are more like alerts, while your notifications are more like an inbox). I'm only wondering if there is a long term vision in this - for example, you when you get a notification, you first see it as an alert, and then it goes to that inbox.

Comment thread web/packages/teleport/src/stores/storeNotifications.ts Outdated
Comment thread web/packages/teleport/src/stores/storeNotifications.ts Outdated
Comment thread web/packages/teleport/src/Navigation/Navigation.tsx Outdated
Comment thread web/packages/teleport/src/TopBar/Notifications/Notifications.tsx Outdated
Comment thread web/packages/teleport/src/TopBar/Notifications/Notifications.tsx Outdated
Comment thread web/packages/teleport/src/TopBar/Notifications/Notifications.tsx Outdated
Comment thread web/packages/teleport/src/TopBar/Notifications/Notifications.tsx Outdated
Comment thread web/packages/teleport/src/TopBar/Notifications/Notifications.tsx Outdated
Comment thread web/packages/teleport/src/TopBar/Notifications/Notifications.tsx Outdated
Comment thread web/packages/teleport/src/stores/storeNotifications.ts Outdated
@kimlisa kimlisa requested a review from gzdunek October 6, 2023 21:41
@kimlisa
Copy link
Copy Markdown
Contributor Author

kimlisa commented Oct 6, 2023

FYI we already have Notifications in shared (you can find them in storybook). We are using them in Connect and Isaiah used them for a desktop session warnings. I'm wondering if these things could (and should) be merged. I also know that our design team is working on a design system for notifications.

Probably these things are different enough that they should be separated (notifications from shared are more like alerts, while your notifications are more like an inbox). I'm only wondering if there is a long term vision in this - for example, you when you get a notification, you first see it as an alert, and then it goes to that inbox.

this seems like a xin/kenny question, but likely it will evolve in the future as we support more types of notification.

gzdunek
gzdunek reviewed Oct 10, 2023
View reviewed changes
Comment thread web/packages/teleport/src/components/Dropdown/Dropdown.tsx Outdated
Comment thread web/packages/teleterm/src/ui/utils/assertUnreachable.ts Outdated
Comment thread web/packages/shared/utils/error.ts Outdated
Comment thread web/packages/teleport/src/stores/storeNotifications.ts Outdated
@kimlisa
Add notification bell to top bar
fb13669 
Also fixes topbar.test act warnings (even though tests passed)
@kimlisa
Add attention dot to navigation switcher
f2ebd12
@kimlisa
Add attention dot to navigation item
3b9c511
@kimlisa
Create store notification and enable it
32210fc
@kimlisa
Address review
d800104 
- Made store notification type more generic
- Drop opactiy styling for top bar drop downs
- Use assertUnreachable in switch cases
@kimlisa
Address CR
491aa73
@kimlisa kimlisa force-pushed the lisa/web/notification-bell branch from 8d421c6 to 491aa73 Compare October 11, 2023 01:28
@kimlisa kimlisa requested a review from gzdunek October 11, 2023 01:30
gzdunek
gzdunek approved these changes Oct 11, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@gzdunek gzdunek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With some minor comments.

Comment thread web/packages/shared/utils/error.ts
Copy link
Copy Markdown
Contributor

@gzdunek gzdunek Oct 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would just call it assertUnreachable.ts

gzdunek
gzdunek reviewed Oct 11, 2023
View reviewed changes
Comment thread web/packages/teleport/src/Discover/Database/CreateDatabase/useCreateDatabase.ts Outdated

import useAttempt from 'shared/hooks/useAttemptNext';
import { getErrMessage } from 'shared/utils/errorType';
import { getErrMessage } from '@gravitational/shared/utils/errorType';
Copy link
Copy Markdown
Contributor

@gzdunek gzdunek Oct 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The import should be from shared/utils/errorType, not @gravitational/shared/utils/errorType.
It should be fixed in other changed files too.

Copy link
Copy Markdown
Contributor Author

@kimlisa kimlisa Oct 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops auto change

@kimlisa kimlisa added this pull request to the merge queue Oct 12, 2023
Merged via the queue into master with commit c53dc1d Oct 12, 2023
@kimlisa kimlisa deleted the lisa/web/notification-bell branch October 12, 2023 18:00
@public-teleport-github-review-bot
Copy link
Copy Markdown

public-teleport-github-review-bot Bot commented Oct 12, 2023

@kimlisa See the table below for backport results.

Branch Result
branch/v13 Create PR
branch/v14 Create PR

This was referenced Oct 12, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gzdunek gzdunek gzdunek approved these changes

@ryanclark ryanclark ryanclark approved these changes

Assignees

No one assigned

Labels

size/lg ui

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@kimlisa @ryanclark @gzdunek