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.

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

feat: update Notifications frontend #23025

Merged
merged 1 commit into from Feb 27, 2024
Merged

feat: update Notifications frontend #23025

merged 1 commit into from Feb 27, 2024

Conversation

mareklibra
Copy link
Contributor

Initial port of Notifications FE from Janus to upstream.

For simpler review, the PR is intentionally kept small in terms of features. The original Janus version is bigger.

On top of this, there will be a serie of small PRs adding:

  • filter on Saved
  • filter on Created
  • pagination
  • sorting
  • severity
  • representatino of Saved (Pinned) notifications - new component
  • reduce filckering (by applying diffs only and by searching on both the FE and BE sides)

Since I have those changes +- ready, there is a bunch of commented-out code left which I am going to use for the next smaller PRs in the upcoming days.
I can remove the commented-out code if it is an issue. Or it will be cleaned by the next PRs comming in a few days.

✔️ Checklist

  • A changeset describing the change and affected packages. (more info)
  • Added or updated documentation
  • Tests for new functionality and regression tests for bug fixes
  • Screenshots attached (for UI changes)
  • All your commits have a Signed-off-by line in the message. (more info)

@mareklibra mareklibra requested review from a team as code owners February 16, 2024 14:25
@backstage-goalie
Copy link
Contributor

backstage-goalie bot commented Feb 16, 2024 •
edited

Changed Packages

Package Name Package Path Changeset Bump Current Version
@backstage/plugin-notifications-backend plugins/notifications-backend minor v0.0.1
@backstage/plugin-notifications-common plugins/notifications-common patch v0.0.1
@backstage/plugin-notifications plugins/notifications minor v0.0.1

@mareklibra
Copy link
Contributor Author

Screencast.from.2024-02-16.15-15-11.webm

@backstage-goalie
Copy link
Contributor

Thanks for the contribution!
All commits need to be DCO signed before they are reviewed. Please refer to the the DCO section in CONTRIBUTING.md or the DCO status for more info.

@mareklibra mareklibra changed the title feat: Update Notifications fronte-end feat: Update Notifications frontend Feb 16, 2024
@mareklibra mareklibra force-pushed the portJanusFE branch 3 times, most recently from 32c993d to ca5747e Compare February 21, 2024 11:08
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 21, 2024 •
edited

Uffizzi Cluster pr-23025 was deleted.

@mareklibra mareklibra changed the title feat: Update Notifications frontend feat: update Notifications frontend Feb 21, 2024
@mareklibra mareklibra force-pushed the portJanusFE branch 3 times, most recently from f03d445 to 8b85f95 Compare February 21, 2024 12:14
@mareklibra
Copy link
Contributor Author

The CI / Verify 18.x is failing on:

## Processing plugins/notifications

Error: The API Report for plugins/notifications is not allowed to have warnings

Error: Process completed with exit code 1.

I do not see the warning in the log.
If I run yarn build:api-reports plugins/notifications locally, no warning/change is reported either.

@Rugvip Any idea, please?

@freben
Copy link
Member

freben commented Feb 21, 2024

@mareklibra if you run it without the last argument (yarn build:api-reports) you get the warning. Sorry about that. It's probably being a bit overly optimistic in optimising the subset of things that it looks at, when you limit the scope.

@mareklibra mareklibra mentioned this pull request Feb 22, 2024
Merged
6 tasks
Rugvip
Rugvip requested changes Feb 26, 2024
View reviewed changes
Copy link
Member

@Rugvip Rugvip left a comment

Choose a reason for hiding this comment

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

Nice! 👍

Tiny changeset update to provide a bit more details about the change, but other than that I think it's best to ship it and keep iterating

.changeset/tender-carrots-care.md Outdated
'@backstage/plugin-notifications-common': patch
---

Notifications frontend has been updated towards expectations of the BEP 001
Copy link
Member

Choose a reason for hiding this comment

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

Better explain the actual change a bit more here, this doesn't really explain the change to users. Doesn't need to be super detailed at this point tbh, just a short snipped about the removal of done state etc.

plugins/notifications/src/components/NotificationsFilters/NotificationsFilters.tsx Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Would prefer not committing commented out code, but if the intention is a quick followup I think it can be alright if it reduces the amount of work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is. There are already some of the PRs pending, blocked on this one.
Based on the speed of reviews, they will be removed quickly.

@mareklibra
feat: update Notifications front-end
758f2a4 
Signed-off-by: Marek Libra <marek.libra@gmail.com>
Rugvip
Rugvip approved these changes Feb 27, 2024
View reviewed changes
Copy link
Member

@Rugvip Rugvip left a comment

Choose a reason for hiding this comment

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

Nice! 🎉 :shipit:

@Rugvip Rugvip merged commit 94c4fe2 into backstage:master Feb 27, 2024
36 checks passed
@github-actions GitHub Actions
Copy link
Contributor

Thank you for contributing to Backstage! The changes in this pull request will be part of the 1.24.0 release, scheduled for Tue, 19 Mar 2024.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@drodil drodil drodil left review comments

@Rugvip Rugvip Rugvip approved these changes

@freben freben Awaiting requested review from freben freben is a code owner automatically assigned from backstage/maintainers

@vinzscam vinzscam Awaiting requested review from vinzscam vinzscam was automatically assigned from backstage/reviewers

@backstage-service backstage-service Awaiting requested review from backstage-service backstage-service is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@mareklibra @freben @drodil @Rugvip