Skip to content
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 notification API for shared libraries #2778

Merged
merged 4 commits into from
Sep 7, 2023
Merged

Add notification API for shared libraries #2778

merged 4 commits into from
Sep 7, 2023

Conversation

cRui861
Copy link
Member

@cRui861 cRui861 commented Aug 9, 2023

Includes a notification demo in the playground for Portal and a mock notifier (for Batch Explorer).

Screenshots from the Portal playground demo:

  • Notification popups.
    image
  • Notifications in the notification tray with the pending notification is in an updated status.
    image
  • Notifications in the notification tray with the pending notification in a completed status.
    image

Screenshots from web Playground demo

  • Using mock notification for alerts (will replace with a Batch Explorer Notifier)
    image
    image

@codecov
Copy link

codecov bot commented Aug 10, 2023

Codecov Report

Merging #2778 (96fb0c4) into main (131fe26) will decrease coverage by 0.02%.
The diff coverage is 59.34%.

❗ Current head 96fb0c4 differs from pull request most recent head a210bf9. Consider uploading reports for the commit a210bf9 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2778      +/-   ##
==========================================
- Coverage   66.47%   66.45%   -0.02%     
==========================================
  Files        1229     1232       +3     
  Lines       33744    33835      +91     
  Branches     6158     6160       +2     
==========================================
+ Hits        22431    22485      +54     
- Misses      11179    11216      +37     
  Partials      134      134              
Files Changed Coverage Δ
packages/playground/src/layout/demo-nav-menu.tsx 100.00% <ø> (ø)
...ges/bonito-core/src/notification/alert-notifier.ts 8.33% <8.33%> (ø)
...ges/playground/src/demo/form/notification-demo.tsx 27.27% <27.27%> (ø)
...es/bonito-core/src/environment/mock-environment.ts 83.78% <50.00%> (-1.94%) ⬇️
packages/playground/src/demo-routes.tsx 56.75% <50.00%> (-0.39%) ⬇️
...ages/bonito-core/src/notification/fake-notifier.ts 84.61% <84.61%> (ø)
...ackages/bonito-core/src/environment/environment.ts 100.00% <100.00%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 131fe26...a210bf9. Read the comment docs.

📢 Have feedback on the report? Share it here.

@gingi
Copy link
Member

gingi commented Aug 16, 2023

Rena, can you provide a screenshot or two of the demo?

Copy link
Member

@gingi gingi left a comment

Choose a reason for hiding this comment

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

The PR looks great, but I think renaming "Notification" to "Notifier" will avoid some confusion in the future.

packages/common/src/ui-common/environment/environment.ts Outdated Show resolved Hide resolved
@cRui861 cRui861 merged commit 1fa572a into main Sep 7, 2023
6 checks passed
@cRui861 cRui861 deleted the rechen/notif branch September 7, 2023 22:38
gingi pushed a commit that referenced this pull request Nov 9, 2023
* WIP Notification fakes

* Move notification files over to bonito-core

* Fix paths

* Update naming from notification to notifier
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants