shahar1 commented on code in PR #76096:
URL: https://github.com/apache/airflow/pull/76096#discussion_r4432070322
##########
airflow-core/src/airflow/ui/src/i18n/ariaLabels.test.ts:
##########
@@ -28,22 +28,26 @@ const sourceFiles: Record<string, string> =
import.meta.glob("../**/*.tsx", {
query: "?raw",
});
-// Only a fully quoted value, with or without braces: aria-label="Next page" or
-// aria-label={"Next page"}. A template literal that interpolates a value
needs a key with
-// its own interpolation, so those are left for a follow-up rather than
exempted here.
-const LITERAL_ARIA_LABEL = /aria-label=\{?"(?<label>[^"]*)"/gu;
+// A fully quoted value, with or without braces: aria-label="Next page" or
aria-label={"Next page"}.
+const LITERAL_ARIA_LABEL = /aria-label=\{?"[^"]*"/gu;
+// A value that is itself a template literal, such as aria-label={`Remove
${name} filter`}. Even
+// when every interpolated part is already translated, building the sentence
here pins English word
+// order, so it belongs in the locale as one key with its own interpolation. A
template literal
+// nested inside a larger expression is not matched:
`translate(`docs.${key}`)` composes a key, not
+// a sentence.
+const TEMPLATE_ARIA_LABEL = /aria-label=\{`[^`]*`\}/gu;
Review Comment:
**minor**: one English `aria-label` is still left, and the widened check
does not catch it.
`pages/Dashboard/Dashboard.tsx:107-111` (unchanged by this PR) builds the
name of the collapse toggle in English:
```tsx
aria-label={
isCollapsed
? `Show ${restAlerts.length} more alert${restAlerts.length === 1 ? "" :
"s"}`
: "Show fewer alerts"
}
```
It hand-rolls the English plural too. Neither pattern matches it:
`LITERAL_ARIA_LABEL` needs the quote right after `aria-label={`, and
`TEMPLATE_ARIA_LABEL` needs the template literal to be the whole value. The
title says these are the *last* English aria-labels, so it would fit here.
`dashboard.json` already has `alerts.showMoreAlerts_one` / `_other` ("+{{count,
number}} more alert(s)"); the toggle needs a sentence-style key with the same
`count` plural, plus one for "Show fewer alerts".
If you would rather keep this PR to the three, please drop "last" from the
title and track the Dashboard toggle in a follow-up. The check could also flag
a string or template literal anywhere in a multi-line `aria-label={ ... }`
value whose text is not inside a `translate(...)` call, but that is optional.
##########
airflow-core/src/airflow/ui/src/pages/TaskInstances/TaskInstancesFilter.test.tsx:
##########
@@ -32,7 +32,7 @@ describe("Task Instances date filters", () => {
const pill = await screen.findByTestId("logical_date_range-pill");
expect(pill).toBeInTheDocument();
- expect(screen.getByRole("button", { name: /Remove .* filter/u
})).toBeInTheDocument();
+
expect(screen.getByTestId("logical_date_range-pill-remove")).toBeInTheDocument();
Review Comment:
**nit**: per the [review
checklist](https://github.com/apache/airflow/blob/main/.github/instructions/code-review.instructions.md#quality-signals-to-check):
*"Flag any existing test modified to accommodate new behavior — this may
indicate a behavioral regression rather than a genuine fix."*
This is not a regression. The only change is that the en wording is now
title case (`Remove Paused Filter`, matching `removeFilter`), so the old
case-sensitive `/Remove .* filter/u` stopped matching. Switching to the test id
drops the accessible-name check, though. Keeping the role query only needs the
case fixed:
```suggestion
expect(screen.getByRole("button", { name: /Remove .* Filter/u
})).toBeInTheDocument();
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
