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

MNTOR-3973 - Announcements UI #5719

Open
wants to merge 28 commits into
base: main
Choose a base branch
from
Open

MNTOR-3973 - Announcements UI #5719

wants to merge 28 commits into from
+1,681 −164

Conversation

codemist
Copy link
Collaborator

@codemist codemist commented Mar 21, 2025 •
edited
Loading

References:

Jira: MNTOR-3973
Figma:

Description

Screenshot (if applicable)

image image image image image

How to test

  • Enable feature flag
    Ensure the Announcements feature flag is enabled for your local environment.

  • Run DB migration
    Run migrations to ensure announcements and user_announcements tables exist and are current.

  • Create announcement
    Go to /admin/announcements and create a new announcement.

    • You don’t need a Fluent ID: non-localized strings will still parse. A “missing fluent id” label will appear in the admin list as a warning. Instructions for testing a localized string are in announcements.ftl or announcements-all.ftl.
    • Make sure it's set to Published or it will not get shown (when you're creating the announcement).

  • Verify in admin
    Confirm your announcement shows up on the admin page.

  • Check dashboard UI
    Go to the dashboard:

    • You should see a badge on the announcement icon showing the number of new announcements.
    • Click the icon and verify the announcement appears under the New tab.
    • Select the item—this should move it to the History tab.
    • Refresh the page; it should remain in History.

  • Sanity Checks

    • Verify user_announcements table has a record for the user with correct status (seen, new, cleared).
    • Shrink the viewport or use dev tools to confirm the announcement icon and modal work on smaller screens.
    • Small updates were made to fix a visual bug where the desktop and mobile navs appeared simultaneously at certain breakpoints. This was fixed by adjusting the mobile breakpoint to prevent both from showing at once.

Checklist (Definition of Done)

  • Localization strings (if needed) have been added.
  • Commits in this PR are minimal and have descriptive commit messages.
  • I've added or updated the relevant sections in readme and/or code comments
  • I've added a unit test to test for potential regressions of this bug.
  • If this PR implements a feature flag or experimentation, I've checked that it still works with the flag both on, and with the flag off.
  • If this PR implements a feature flag or experimentation, the Ship Behind Feature Flag status in Jira has been set
  • Product Owner accepted the User Story (demo of functionality completed) or waived the privilege.
  • All acceptance criteria are met.
  • Jira ticket has been updated (if needed) to match changes made during the development process.
  • Jira ticket has been updated (if needed) with suggestions for QA when this PR is deployed to stage.

Sorry, something went wrong.

@codemist codemist added WIP 🚧 Work in Progress 🛑 Do Not Merge Do not merge this PR, even if approved. labels Mar 21, 2025
@codemist codemist removed WIP 🚧 Work in Progress 🛑 Do Not Merge Do not merge this PR, even if approved. labels Mar 28, 2025
@codemist codemist marked this pull request as ready for review March 28, 2025 14:44
@codemist codemist requested a review from flodolo as a code owner March 28, 2025 14:44
@codemist codemist changed the title MNTOR-3973 - Announcements UI (WIP) MNTOR-3973 - Announcements UI Mar 28, 2025
@codemist codemist requested review from Vinnl and rhelmer March 28, 2025 14:51
@github-actions GitHub Actions
Copy link

Preview URL 🚀 : https://blurts-server-pr-5719-mgjlpikfea-uk.a.run.app

codemist
codemist commented Mar 28, 2025
View reviewed changes
src/app/components/client/toolbar/AppPicker.module.scss
@@ -39,7 +39,7 @@
content: "";
height: $tooltip-size;
position: absolute;
right: 100px; // Vertically align chevron with bento icon
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I noticed that this was actually buggy on prod:
image

I fixed it here.

codemist
codemist commented Mar 28, 2025
View reviewed changes
src/app/components/client/toolbar/UserMenu.module.scss
@@ -12,10 +12,6 @@ $button-size: 42px;
width: $button-size;
overflow: hidden;

@media screen and (max-width: tokens.$screen-md) {
Copy link
Collaborator Author

@codemist codemist Mar 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Changes clean-up some of the padding inconsistencies, which I noticed when I introduced the bell announcement icon.

flodolo
flodolo requested changes Mar 29, 2025
View reviewed changes
locales/en/announcements-all.ftl
# License, v. 2.0. If a copy of the MPL was not distributed with this
# file, You can obtain one at http://mozilla.org/MPL/2.0/.

# between announcement- and -title or -description is the announcement_id.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This comment is not really relevant for this file at the moment.

locales/en/announcements-all.ftl

# between announcement- and -title or -description is the announcement_id.

announcement-dialog-default-tab = New
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
announcement-dialog-default-tab = New
# Used as selector at the top of a dialog to only display new announcements.
announcement-dialog-default-tab = New

We need to add context in a comment, as localizers won't have access to the design. Is what I suggested accurate?

locales/en/announcements-all.ftl
# between announcement- and -title or -description is the announcement_id.

announcement-dialog-default-tab = New
announcement-dialog-history-tab = All
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
announcement-dialog-history-tab = All
# Used as selector at the top of a dialog to display all announcements.
announcement-dialog-history-tab = All

locales/en/announcements-all.ftl
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we planning to expose announcements to locales other than English? If no, I assume we shouldn't expose this for localization.

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

@flodolo flodolo

@Vinnl Vinnl

@rhelmer rhelmer

Requested changes must be addressed to merge this pull request.

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
2 participants
@codemist @flodolo