MNTOR-4298 - Announcements ADR #5866

codemist · 2025-05-07T18:09:17Z

References:

Description

Introduces a Fluent UI section and accompanying documentation to the Announcements admin page, giving Product and QA teams clearer context and improving usability for this feature.

Screenshot (if applicable)

Not applicable.

How to test

Checklist (Definition of Done)

github-actions · 2025-05-07T18:29:33Z

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

codemist · 2025-05-08T13:47:57Z

package.json

@@ -78,6 +78,7 @@
    "@fluent/bundle": "^0.19.1",
    "@fluent/langneg": "^0.7.0",
    "@fluent/react": "^0.15.2",
+    "@fluent/syntax": "^0.19.0",


Pretty safe dependency built for tooling around fluent:

It’s designed to parse, analyze, process, and serialize Fluent files.

That link goes to the docs for the Python implementation of Fluent - I think you mean this one :)

src/app/(proper_react)/(redesign)/(authenticated)/admin/announcements/getFluentStrings.ts

flozia · 2025-05-27T11:47:06Z

src/app/(proper_react)/(redesign)/(authenticated)/admin/announcements/AnnouncementsAdmin.tsx

@@ -31,6 +33,8 @@ export const AnnouncementsAdmin = (props: Props) => {
  const [activeAnnouncementToEdit, setActiveAnnouncementToEdit] =
    useState<AnnouncementRow | null>(null);

+  console.log(props.fluentStrings);


Suggested change

console.log(props.fluentStrings);

flozia · 2025-05-27T11:54:37Z

src/app/(proper_react)/(redesign)/(authenticated)/admin/announcements/AnnouncementsAdmin.tsx

+                          alt="Fallback image"
+                          width={500}
+                          height={300}
+                          key={activeAnnouncement.id}


The keys of the small and big images are duplicates: To make sure they don’t collide we should make them unique with for example:

Suggested change

key={activeAnnouncement.id}

key={`${activeAnnouncement.id}-small-fallback`}

key={`${activeAnnouncement.id}-small`}

key={`${activeAnnouncement.id-big-fallback`}

key={`${activeAnnouncement.id}-big`}

flozia · 2025-05-27T11:56:57Z

src/app/(proper_react)/(redesign)/(authenticated)/admin/announcements/AnnouncementsAdmin.tsx

+                      {bigImageUnavailable ? (
+                        <Image
+                          alt="Fallback image"
+                          width={500}
+                          height={300}
+                          key={activeAnnouncement.id}
+                          className={styles.bigImage}
+                          src="/images/announcements/fallback/big.svg"
+                          onLoadingComplete={() => setBigImageIsLoading(false)}
+                        />
+                      ) : (
+                        <Image
+                          alt="Announcement preview"
+                          width={500}
+                          height={300}
+                          key={activeAnnouncement.id}
+                          src={bigImagePath}
+                          className={styles.bigImage}
+                          onLoadingComplete={() => setBigImageIsLoading(false)}
+                          onError={() => setBigImageUnavailable(true)}
+                        />
+                      )}


Suggestion (non-blocking): The fallback image logic is used a couple of times — it would be nice if we could unify this into a reusable component.

36c6d22 Added ImageWithFallback which handles repeated logic.

flozia · 2025-05-27T11:59:17Z

...(proper_react)/(redesign)/(authenticated)/admin/announcements/AnnouncementsAdmin.module.scss

+      list-style: none;
+      margin-top: tokens.$spacing-md;
+      padding-left: 0;


For list resetting, we have the class .noList available that you could add to the <ul>.

flozia · 2025-05-27T12:02:05Z

src/app/(proper_react)/(redesign)/(authenticated)/admin/announcements/getFluentStrings.ts

+  const ftlPath = path.resolve("locales-pending/announcements.ftl");
+  const raw = fs.readFileSync(ftlPath, "utf8");
+  const resource: Resource = parse(raw, { withSpans: false });


Suggestion: This part should be wrapped in a try {} catch {} to handle missing or invalid file contents when parsing the file.

flozia · 2025-05-27T12:10:28Z

src/app/(proper_react)/(redesign)/(authenticated)/admin/announcements/getFluentStrings.ts

+      if (field === "title") {
+        result[id].title = value;
+      } else if (field === "description") {
+        result[id].description = value;
+      } else if (field === "cta-label") {
+        result[id].ctaLabel = value;
+      }


Question: Instead of mapping the string IDs could we also leave them as they are (in kebab-case)? If we still need to map them: Since we are not handling any additional logic we could also map them like this:

Suggested change

if (field === "title") {

result[id].title = value;

} else if (field === "description") {

result[id].description = value;

} else if (field === "cta-label") {

result[id].ctaLabel = value;

}

const map: Record<string, keyof AnnouncementGroup> = {

title: "title",

description": "description",

"cta-label": "ctaLabel",

};

result[id][map[field]] = value;

… or maybe parse the IDs to camelCase if we expect them to match otherwise.

flozia · 2025-05-27T12:13:31Z

src/app/(proper_react)/(redesign)/(authenticated)/admin/announcements/getFluentStrings.ts

Even when getFluentStrings is only used for the announcements admin panel, this function would benefit from some initial unit tests to validate the expected behavior. This would be especially helpful when making changes in the future.

codemist · 2025-06-17T14:38:45Z

src/app/(proper_react)/(redesign)/(authenticated)/admin/announcements/AnnouncementsAdmin.tsx

+import { GroupedFluentAnnouncements } from "./getFluentStrings";
+import { ImageWithFallback } from "./ImageWithFallback";
+import { FluentStringsView } from "./FluentStringsView";
+import { AnnouncementsDocsView } from "./DocsView";


This file was getting kind of unwieldy - I split out the tabs into separate components.

Vinnl

I started out looking at the changed code, but it was so much, is never exposed to users, and it seems like Florian has also already done a pass on that, so I opted to stop my review of that and leave it to Florian to finish that up on Monday. I will say, though, that I'm impressed that you dove into the depths of the available Fluent packages to be able to display an interface over them :)

Speaking of, that interface looks useful, and having the docs there in context is as well. (Though small nit: it does not Record any Architectural Decisions, so it's not an ADR.) My main suggestion, though, would be to split it up in two: docs for admins using the UI (these docs), and docs for the parts that need to be done by an engineer, which could just live as a Markdown file under /docs/ in the repo (with a reference to it from the UI). Because right now, I imagine Tony would be a bit lost when trying to follow these docs.

Vinnl · 2025-06-19T13:18:15Z

package.json

@@ -78,6 +78,7 @@
    "@fluent/bundle": "^0.19.1",
    "@fluent/langneg": "^0.7.0",
    "@fluent/react": "^0.15.2",
+    "@fluent/syntax": "^0.19.0",


That link goes to the docs for the Python implementation of Fluent - I think you mean this one :)

Vinnl · 2025-06-19T13:22:11Z

...(proper_react)/(redesign)/(authenticated)/admin/announcements/AnnouncementsAdmin.module.scss

I don't care so much because it's just an admin page, but I always do a double-take whenever I type margin, which I can highly recommend (just like e.g. for float) 🙂 There's probably times when they're justified, but most of the time they make the layout of the rest of the page unpredictable over time.

Vinnl · 2025-06-19T13:30:34Z

src/app/(proper_react)/(redesign)/(authenticated)/admin/announcements/AnnouncementsAdmin.tsx

-              >
-                <div>
-                  <p className={styles.title}>
+      <div className={styles.tabBar}>


Again, not a blocker since it's "just" an admin page, but react-aria implements tab behaviour - e.g. to make sure you can switch tabs using the arrow keys, and to announce them to screen readers. Just FYI for the future.

flozia · 2025-06-26T13:13:15Z

src/app/(proper_react)/(redesign)/(authenticated)/admin/announcements/AnnouncementsAdmin.tsx

+                <button
+                  className={styles.addButton}
+                  onClick={() => setIsModalOpen(true)}
+                >
+                  + Add new announcement
+                </button>


A <button> as a direct child of <ul> is not valid and would need to be wrapped in an <li>.

flozia · 2025-06-26T13:15:36Z

src/app/(proper_react)/(redesign)/(authenticated)/admin/announcements/AnnouncementsAdmin.tsx

+                  <dt>Big Image</dt>
+                  <dd>
+                    <ImageWithFallback
+                      src={smallImagePath}


Should this be:

Suggested change

src={smallImagePath}

src={bigImagePath}

?

flozia

Considering this is for admins only and the purpose of these additions, it looks good. There are some open pending comments that deserve a response before finishing this PR up.

codemist commented May 8, 2025

View reviewed changes

codemist marked this pull request as ready for review May 14, 2025 14:05

codemist requested a review from flozia May 14, 2025 14:05

flozia reviewed May 27, 2025

View reviewed changes

codemist added 5 commits June 17, 2025 04:18

add getfluentstrings file and add fluent/syntax lib

61257ec

add search filter

8a8dee7

disable state for first announcement string group

bcf05f1

add announcements adr

615f020

del unnecessary file

2255f5b

codemist force-pushed the mntor-4298-1 branch from 706c58f to 2255f5b Compare June 17, 2025 08:18

codemist added 9 commits June 17, 2025 04:43

remove console log

0492fa7

unique keys for img

f309d20

add fallback component

36c6d22

use noList global class

35e50a2

ftlpath try/catch

9be60de

organize different tab states

03b1c78

parse ftl strings into grp announcement data

0d62572

lint

3f265dc

Merge branch 'main' into mntor-4298-1

2cae8ee

codemist commented Jun 17, 2025

View reviewed changes

codemist requested a review from Vinnl June 18, 2025 13:19

Vinnl reviewed Jun 19, 2025

View reviewed changes

codemist requested a review from flozia June 24, 2025 14:51

flozia reviewed Jun 26, 2025

View reviewed changes

-                          key={activeAnnouncement.id}
+                          key={`${activeAnnouncement.id}-small-fallback`}
+                          key={`${activeAnnouncement.id}-small`}
+                          key={`${activeAnnouncement.id-big-fallback`}
+                          key={`${activeAnnouncement.id}-big`}

MNTOR-4298 - Announcements ADR #5866

Are you sure you want to change the base?

MNTOR-4298 - Announcements ADR #5866

Conversation

codemist commented May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

References:

Description

Screenshot (if applicable)

How to test

Checklist (Definition of Done)

Uh oh!

github-actions bot commented May 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Vinnl left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flozia left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codemist commented May 7, 2025 •

edited

Loading

Vinnl left a comment •

edited

Loading