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

fix(notifications): handle overflow #761

Merged
merged 22 commits into from
Dec 19, 2022

Conversation

andrewazores
Copy link
Member

Welcome to Cryostat! 👋

Before contributing, make sure you have:

  • Read the contributing guidelines
  • Linked a relevant issue which this PR resolves
  • Linked any other relevant issues, PR's, or documentation, if any
  • Resolved all conflicts, if any
  • Rebased your branch PR on top of the latest upstream main branch
  • Attached at least one of the following labels to the PR: [chore, ci, docs, feat, fix, test]
  • Signed the last commit: git commit --amend --signoff

Fixes: #757

Description of the change:

This change adds overflow handling to the notification alert group and notification drawer.

https://www.patternfly.org/v4/components/alert-group#toast-alert-group-with-overflow-capture

Motivation for the change:

This reduces visual clutter on the screen and keeps the UI from feeling overfilled and chaotic when many actions are happening within a short timeframe. It is also easier for the user to dismiss all notifications when there are very many since the notification drawer can be summoned in a second way (clicking the overflow message), and summoning the drawer also hides the toast boxes.

How to manually test:

There is a new temporary plus button on the top bar to help testing. This is to be removed before merge. Clicking it adds a new notification immediately.

Generate notifications with the temporary button or by any other means and observe the Overflow message that appears when there are too many notifications. Go to the Settings view and try changing the visible notifications count to various values and repeat.

@github-actions
Copy link

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-761-fac7584578bbd87394748f310ac7a3a56d308bd8 sh smoketest.sh

@github-actions
Copy link

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-761-1dd312202570586c79322cc517df6a010e607544 sh smoketest.sh

@andrewazores andrewazores marked this pull request as ready for review December 15, 2022 19:52
@andrewazores andrewazores requested a review from tthvo December 15, 2022 19:52
@github-actions
Copy link

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-761-191bcf0e5f13ddcb8fd3f00ce6697a497d7e21a6 sh smoketest.sh

@github-actions
Copy link

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-761-a2cfa19a7d8b6e7d5d5f321756ff95e102b85d75 sh smoketest.sh

Copy link
Member

@tthvo tthvo left a comment

Choose a reason for hiding this comment

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

Looks nicee! Just some comments :D

@github-actions
Copy link

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-761-bf2b444bb730f01a01d12120d3a72d6bdcc2a744 sh smoketest.sh

@github-actions
Copy link

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-761-300611584e6fbd9082d998ce4f1d66fb49f0facb sh smoketest.sh

@github-actions
Copy link

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-761-18859f67423bad4b85bc883927df6fc0a5471890 sh smoketest.sh

@github-actions
Copy link

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-761-981f785a1e6fc292f8748420cad80022b1f4808a sh smoketest.sh

@tthvo
Copy link
Member

tthvo commented Dec 16, 2022

Just a small detail:

If I have the drawer open, and notifications come in. I see them in the list and the top-right pop-ups are hidden.

Screenshot from 2022-12-16 14-20-56

If I close the notification drawer, the pop-ups appear:

Screenshot from 2022-12-16 14-21-02

Do you think if any un-read notifications that appear while the drawer is open should be hidden by timing-out early? Not sure if we can do anything about that.

@andrewazores
Copy link
Member Author

Hmm. Not sure it's worth bothering to handle. I guess it could be done by checking if the drawer is open when new notifications are added and setting the hidden: true property immediately if so, but the service that handles the notification stream doesn't currently know the state of the drawer. Maybe it should? Opening the drawer could auto-hide all of the notifications so they don't re-appear when the drawer is closed again, regardless of if the notifications were there before or came in while the drawer was open.

@github-actions
Copy link

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-761-b6b25b6345455aaef471e4becc320c96c8313b9d sh smoketest.sh

@tthvo
Copy link
Member

tthvo commented Dec 16, 2022

Maybe it should? Opening the drawer could auto-hide all of the notifications so they don't re-appear when the drawer is closed again, regardless of if the notifications were there before or came in while the drawer was open.

Yeh agreed. Do you think this PR can include these changes?

@andrewazores
Copy link
Member Author

Sure, I'll look at it now.

@github-actions
Copy link

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-761-73f67081c0e3351d2728af744b8819872a27c1a1 sh smoketest.sh

@github-actions
Copy link

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-761-c379d660ee05e91a0b4335b126bff324117462ec sh smoketest.sh

@github-actions
Copy link

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-761-0e4ad8a9eed8db588647ecf04aaddc4ba0262305 sh smoketest.sh

@github-actions
Copy link

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-761-f48186979356dddd64d31e7347e5e551dde3c5e1 sh smoketest.sh

@github-actions
Copy link

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-761-15e4606db8e6b4471b6cdfaca422ac1abc91e8db sh smoketest.sh

@tthvo
Copy link
Member

tthvo commented Dec 19, 2022

Seems like ci failed the first time with the exact test failure: https://github.com/cryostatio/cryostat-web/actions/runs/3734294205/jobs/6336169378 as described in #661 (comment). Cant reproduce it very consistently tho, but seems to occur more frequently lately.

Copy link
Member

@tthvo tthvo left a comment

Choose a reason for hiding this comment

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

Looks great to me!

@andrewazores andrewazores merged commit c7133b4 into cryostatio:main Dec 19, 2022
@andrewazores andrewazores deleted the notification-overflow branch December 19, 2022 19:31
@github-actions
Copy link

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-761-15e4606db8e6b4471b6cdfaca422ac1abc91e8db sh smoketest.sh

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Bug] Notifications can overwhelm the screen and cover the notification drawer
2 participants