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

Implement Notifications API for templates #3093

Merged
merged 7 commits into from
Jan 13, 2022
Merged

Implement Notifications API for templates #3093

merged 7 commits into from
Jan 13, 2022

Conversation

philippjfr
Copy link
Member

@philippjfr philippjfr commented Jan 11, 2022

Adds pn.state.notifications which can be enabled with pn.config.notifications = True | False.

Demo

notifications_nb

ToDo

  • Finalize API after feedback
  • Decide on styling API
  • Add docs
  • Add tests

@philippjfr
Copy link
Member Author

@maximlt @MarcSkovMadsen @jbednar This is very much a work in progress but if you have comments/suggestions about the API that would be very helpful.

@philippjfr philippjfr added this to the v0.13.0 milestone Jan 11, 2022
@MarcSkovMadsen
Copy link
Collaborator

My thought was that I would have added the toast as a new component. I made an example on discourse a while back that shows the concept. I think it's more flexible not to tie it to a template, but provide the toast as a basic building block.

@jbednar
Copy link
Member

jbednar commented Jan 12, 2022

Looks good to me!

Like Marc, I do think it might not be necessary to tie it to a template. E.g. couldn't we have the same app in a notebook, but then displaying the notification some other way?

@MarcSkovMadsen
Copy link
Collaborator

MarcSkovMadsen commented Jan 12, 2022

The request for a standalone toast is here #2802

The basic implementation is here https://discourse.holoviz.org/t/toast-a-pop-up-notification/2903

I would really like a Toast widget based on Notyf JS. That is simple and beautiful. And I would also be willing to do the PR.

@MarcSkovMadsen
Copy link
Collaborator

MarcSkovMadsen commented Jan 12, 2022

One question I have is whether Toasts should be layouts that can display Panel components? The toast I proposed can only display text/ html. That is my need.

@philippjfr
Copy link
Member Author

The reason why I think it makes sense as part of a template or as a global object is that it doesn't respect any of the layout placement you might otherwise do with it, e.g. placing it inside a Row/Column effectively makes no sense, because the actual toasts will appear in the fixed absolute position you specify. Adding it as a root with .servable does make sense I suppose and you can still do that with my version.

If we came up with some UI that worked in notebooks I'd probably just move the notifications object onto pn.state.

@philippjfr
Copy link
Member Author

Okay, so now I'm leaning towards actually making it global, i.e. providing a pn.config.notifications option to enable the notification area and then store the object on pn.state.notifications.

@philippjfr
Copy link
Member Author

Okay based on the feedback I'm now leaning towards the pn.config.notifications and pn.state.notifications option. Enabling the option will enable notifications both in the notebook and in the served app:

notifications_nb

@MarcSkovMadsen
Copy link
Collaborator

Seems like a good solution to me? Should the modal one day be similarly refactored into a global option?

@philippjfr
Copy link
Member Author

Seems like a good solution to me? Should the modal one day be similarly refactored into a global option?

If we can make a modal that works in the notebook I'd be very open to that.

@codecov
Copy link

codecov bot commented Jan 13, 2022

Codecov Report

Merging #3093 (649c401) into master (afd555a) will decrease coverage by 0.28%.
The diff coverage is 32.09%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3093      +/-   ##
==========================================
- Coverage   83.46%   83.17%   -0.29%     
==========================================
  Files         190      192       +2     
  Lines       25206    25414     +208     
==========================================
+ Hits        21038    21138     +100     
- Misses       4168     4276     +108     
Impacted Files Coverage Δ
panel/io/notifications.py 0.00% <0.00%> (ø)
panel/template/fast/base.py 96.47% <ø> (ø)
panel/template/material/__init__.py 100.00% <ø> (ø)
panel/config.py 61.29% <33.33%> (-0.76%) ⬇️
panel/io/state.py 69.28% <33.33%> (-1.86%) ⬇️
panel/io/datamodel.py 47.91% <47.91%> (ø)
panel/template/base.py 78.43% <60.00%> (-0.28%) ⬇️
panel/io/resources.py 82.88% <100.00%> (+0.15%) ⬆️
panel/links.py 85.62% <100.00%> (+7.62%) ⬆️
panel/models/plotly.py 96.66% <100.00%> (ø)
... and 12 more

Continue to review full report at Codecov.

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

@philippjfr philippjfr merged commit 7d21b73 into master Jan 13, 2022
@philippjfr philippjfr deleted the notifications branch January 13, 2022 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants