Skip to content

feat: add support for multiple banners #13081

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
May 8, 2024
Merged

feat: add support for multiple banners #13081

merged 7 commits into from
May 8, 2024

Conversation

aslilac
Copy link
Member

@aslilac aslilac commented Apr 25, 2024 •
edited
Loading

Replace the singleton "service banner" with "notification banners" that can support any number of banners

Screenshot 2024-05-02 at 3 28 18 PM
  • Migrate any existing banner to be a notification banner
  • Rip out service banner logic/have it read from the new state depending on what makes sense
  • Updated frontend for editing the notification banners state
  • Remove a bunch of weird "preview" stuff, and just show the preview while you're editing

@aslilac aslilac force-pushed the notification-banners branch from f6c1618 to d12bdf4 Compare April 25, 2024 22:33
@aslilac

This comment was marked as resolved.

Sign in to view
@aslilac aslilac requested review from BrunoQuaresma and Emyrk May 3, 2024 23:53
Emyrk
Emyrk reviewed May 6, 2024
View reviewed changes
@aslilac aslilac changed the title feat: add support for custom notification banners feat: add support for multiple banners May 6, 2024
@aslilac aslilac force-pushed the notification-banners branch from a0a6cce to 71c06e0 Compare May 6, 2024 18:45
@aslilac aslilac marked this pull request as ready for review May 6, 2024 19:13
@aslilac aslilac requested a review from Emyrk May 6, 2024 19:18
BrunoQuaresma

This comment was marked as resolved.

Sign in to view

Emyrk
Emyrk approved these changes May 7, 2024
View reviewed changes
Copy link
Member

@Emyrk Emyrk left a comment

Choose a reason for hiding this comment

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

BE LG 👍

spikecurtis

This comment was marked as resolved.

Sign in to view

@aslilac aslilac requested a review from spikecurtis May 7, 2024 18:27
@aslilac aslilac requested a review from BrunoQuaresma May 7, 2024 20:00
BrunoQuaresma
BrunoQuaresma approved these changes May 8, 2024
View reviewed changes
Copy link
Collaborator

@BrunoQuaresma BrunoQuaresma 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!

Just a nit but it is up to you. Having the banner getting added to the table when the dialog is open is a bit unexpected but anything that compromises the usage or experience.

@aslilac aslilac merged commit d8e0be6 into main May 8, 2024
31 checks passed
@aslilac aslilac deleted the notification-banners branch May 8, 2024 21:40
@github-actions github-actions bot locked and limited conversation to collaborators May 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@BrunoQuaresma BrunoQuaresma BrunoQuaresma approved these changes

@spikecurtis spikecurtis spikecurtis approved these changes

@Emyrk Emyrk Emyrk approved these changes

Assignees

@aslilac aslilac

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@aslilac @BrunoQuaresma @spikecurtis @Emyrk