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 queue and display using toast #12959

Merged
merged 34 commits into from
Oct 13, 2022

Conversation

telamonian
Copy link
Member

@telamonian telamonian commented Aug 17, 2022

Adds toast ui to jupyterlab. I've taken @fcollonval's code from jupyterlab_toastify and merged it into ui-components. Currently non-functional draft.

References

Fixes #689
#7980
#12827

Code changes

  • Add NotificationManager; aka a notification queue with a changed signal to be notified
  • Add a namespace Notification to access to the manager (discourage for final user) and helper functions to emit notifications of various type (success, warning, error, info, in-progress or default) and helpers to update and dismiss a notification.
    This is accessible globally from @jupyterlab/apputils - it does not require to request token in plugins.
  • Add a new plugin @jupyterlab/apputils-extension:notification that
    • o Add commands apputils:notify to emit a notification and apputils:dismiss to dismiss a notification
    • o Add a status bar widget to display the number of notifications
    • o If a autoClose option is set to a positive number, a toast will be emitted. Otherwise the notification will be accessible by the user when clicking on the status bar widget (the goal is to limit disturbance of pop ups to the user)
    • When clicked on the status bar widget, all notifications are displayed
    • Allow for markdown in message
    • Add a settings to force all notification to be silent

To have an idea of the toast style, you can look at the newly uploaded test snapshots in the Files changed tab.

User-facing changes

  • New bell icon in the status bar
  • User may be notified via toast

Backwards-incompatible changes

N/A

@jupyterlab-probot
Copy link

Thanks for making a pull request to jupyterlab!
To try out this branch on binder, follow this link: Binder

@krassowski
Copy link
Member

krassowski commented Aug 17, 2022

API: re-surfacing a comment from #689 (comment) and re-phrasing it to: should we use the same buttons API as in IDialog, or have a separate definition as in the initial draft?

buttons: ReadonlyArray<IButton>;

@telamonian
Copy link
Member Author

@krassowski I do like the idea of standardizing our many button interfaces

@ellisonbg
Copy link
Contributor

@telamonian Thanks for the PR. I am fine with us implementing the full notification system incrementally, and I think this capability is a good start. At the same time, APIs are difficult to change. Have you (or can you) sync with @afshin and @andrii-i about the work they have done on the broader notification system and the API requirements therein? Best case scenario, any additional APIs needed could be added during the 4.0 release cycle without breaking anything. I think they were using parts of jupyterlab_toastify so hopefully it will be easy to align.

@fcollonval
Copy link
Member

Notes for the integration:

  • We should not use font awesome but prefer to bring the needed icons in core.
  • We should remove notify L337 to avoid binding us to much with the underlying library
  • It will be nice to be able to provide a custom renderer as available for dialog for example.

I am fine with us implementing the full notification system incrementally

👍 for example a follow-up could be to add a manager as middle-man between the notification interface and the display (to allow for history or for filtering).

API: re-surfacing a comment from #689 (comment) and re-phrasing it to: should we use the same buttons API as in IDialog, or have a separate definition as in the initial draft?

Although it will be nice to avoid separating those definitions, I don't think it will be possible. The main reason is the behavior difference between a dialog and a notification. In the dialog case, it prompts the user to do immediate action blocking other UI elements. For that our API uses a promise returning the result of the dialog. This will not fit for a toast that has a behavior emit a message and go-on. Hence the use of callback in the notification button interface.
We could change the dialog logic to use callback. But that will have a high impact for downstream extensions (and lots work in core itself).

@fcollonval fcollonval mentioned this pull request Sep 12, 2022
8 tasks
@fcollonval
Copy link
Member

@telamonian I have taken the freedom to push this one to be able to get it into 3.5.

@fcollonval fcollonval changed the title pulled code from jupyterlab_toastify into ui-components Add notification queue and display using toast Sep 23, 2022
@fcollonval
Copy link
Member

bot please update snapshots

@fcollonval
Copy link
Member

bot please update documentation snapshots

@fcollonval
Copy link
Member

bot please update galata snapshots

@fcollonval
Copy link
Member

bot please update galata snapshots

@fcollonval
Copy link
Member

Kicking CI

@fcollonval fcollonval closed this Oct 12, 2022
@fcollonval fcollonval reopened this Oct 12, 2022
Copy link
Member

@krassowski krassowski left a comment

Choose a reason for hiding this comment

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

We discussed this at the team meeting today, the question of returning a string vs an IDisposable was deferred to a follow up PR with good arguments for having a string (allowing other actors to act on notification e.g. in RTC context) and for IDisposable (type safety, developer convenience).

This PR currently uses a string with ID but it can be changed in a follow up PR before 4.0 release if needed.

Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Thanks all for the reviews

@fcollonval fcollonval merged commit 8c93347 into jupyterlab:master Oct 13, 2022
export const notificationPlugin: JupyterFrontEndPlugin<void> = {
id: '@jupyterlab/apputils-extension:notification',
autoStart: true,
requires: [IStatusBar],
Copy link
Member

Choose a reason for hiding this comment

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

Is IStatusBar really required for this plugin? Or could it also be made optional?

Or move the status bar related logic to a separate plugin so the commands are still available.

Copy link
Member

Choose a reason for hiding this comment

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

The only UI entry point for the notification is a status bar widget - as for the log console.

I don't know what would be the best solution for Notebook v7 - as we should promote not disturbing the user with many toasts.

This definitely needs to be discussed for notebook v7 (including the log console that I guess is not loaded neither for now).

Copy link
Member

Choose a reason for hiding this comment

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

If the status bar is the only part of the UI where notifications are displayed for now then Notebook 7 can just ignore for now.

Copy link
Member

Choose a reason for hiding this comment

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

Although it would be great to have to leverage announcements features in nb7 like #13365

@fcollonval
Copy link
Member

@meeseeksdev please backport to 3.6.x

@lumberbot-app
Copy link

lumberbot-app bot commented Oct 31, 2022

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
git checkout 3.6.x
git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
git cherry-pick -x -m1 8c93347cdd57c0ca20197a2f4eabbc43f2ab959e
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #12959: Add notification queue and display using toast'
  1. Push to a named branch:
git push YOURFORK 3.6.x:auto-backport-of-pr-12959-on-3.6.x
  1. Create a PR against branch 3.6.x, I would have named this PR:

"Backport PR #12959 on branch 3.6.x (Add notification queue and display using toast)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

fcollonval pushed a commit to fcollonval/jupyterlab that referenced this pull request Oct 31, 2022
fcollonval pushed a commit to fcollonval/jupyterlab that referenced this pull request Nov 3, 2022
fcollonval added a commit that referenced this pull request Nov 4, 2022
…y using toast) (#13360)

* Backport PR #12959: Add notification queue and display using toast

* Update Playwright Snapshots

* Fix button styles

* Update Playwright Snapshots

* markedparser-extension does not exists in 3.x

Co-authored-by: Max Klein <telamonian@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create proper notification system and area - Toast Notifications
6 participants