Repository navigation
Translate the line a screen reader says before each announcement - #654
Merged
Merged
Conversation
@wordpress/a11y writes it at DOM-ready, before the renderer has a locale, so it stayed English in every language. It is written again once the locale is applied, beside the window's title. Fixes #648.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 38 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Collaborator
Author
|
@coderabbitai review |
|
ryanwelcher
added a commit
that referenced
this pull request
Oct 8, 2026
…wn (#660) ## Why Every pull request runs the unit suite and the E2E journeys and packaged smoke on macOS and Windows, and all six checks are required to merge. A pull request that only changes Markdown (README, RELEASING.md, AGENTS.md and the like) waits on all of them even though nothing they test has changed. ## What changes A pull request whose changed files all end in `.md` skips the unit suite, the journeys and the packaged smoke, and still merges. `paths-ignore` cannot do this. A workflow it skips never reports its checks, so the required ones would sit at "pending" and block the merge. Instead, each workflow gets a small `changes` job on Linux that reads the PR's file list from the API. When only Markdown changed, `*-skipped` stub jobs report the same required check names as passed, and the real jobs are skipped. Pushes to trunk always run everything. Deliberately not in it: anything under `docs/` that is not Markdown (`tests/unit/download-button-platform.test.cjs` reads `docs/.vitepress/theme`), screenshots, and Lint, which is quick and also required. ## How to test this Platforms: none, this is CI only. The checks on this PR and the next Markdown-only PR are the test. **Starting state:** this PR, which changes two workflow files. 1. Look at the checks on this PR. Expected: `changes` reports `code=true`, and the real `unit`, `Journeys` and `Packaged smoke` jobs run on macOS and Windows. 2. On a Markdown-only PR opened after this merges (or a throwaway one against this branch), look at the checks. Expected: the six required checks show as passed within about a minute, each from a job that prints "Only Markdown changed", and no macOS or Windows runner starts. 3. Edit that PR's title. Expected: a new run appears with every job skipped, and the six required checks keep their earlier passes. **What must not have happened:** - A PR with any non-Markdown file skipping a suite. The file test was checked by hand for Markdown only, Markdown plus `.js`, a VitePress config change, an empty list and a `.js` file renamed to `.md`, and against the real file lists of #546 (Markdown only, skips) and #654 (code, runs). - A required check left at "pending" on a Markdown-only PR. - A title edit cancelling a suite that is still running. **Cannot be tested here:** the base-move case below needs a stacked pair of PRs to retarget, which I did not stage. ## Risks and limitations - If the `changes` job fails (for example the API call errors), both the real and the stub jobs skip, so the required checks stay pending. That blocks the merge rather than letting it through, and re-running the workflow clears it. - Moving a PR's base re-runs `changes`. Without that, a Markdown-only PR stacked on a code PR and moved onto trunk by hand would keep its stub passes while its diff now contains the lower PR's code. The workflows now listen for `edited` and act only when the base changed. One unverified detail: whether the PR's file list already reflects the new base when the `edited` run starts. - The `changes` script is copied into both workflows rather than shared. Sharing it would mean a composite action for twenty lines. - Pre-PR review: 1 finding, fixed (see below). ## Related None. --- <details> <summary>Design decisions and alternatives considered</summary> - **`paths-ignore` / `paths`:** rejected, the required checks would never report. - **A job-level `if:` on the existing matrix jobs alone:** rejected. A skipped matrix job is not expanded, so it reports one check named literally `Journeys (${{ matrix.os }})`, and the required `Journeys (macos-latest)` never appears. This is already what happens to drafts today (see #283's checks). - **A step-level `if:` on every step of the real jobs, with `runs-on` falling back to Linux:** works, but every new step would need the guard, and a missed one would run `npm ci` on the wrong runner. The stub jobs keep the real jobs unchanged apart from `needs` and `if`. - **Changing the ruleset to require one gate job per workflow:** needs an admin change to the ruleset and moves the required names; the stubs need neither. - **`dorny/paths-filter`:** a third-party action holding a token, for something `gh api` does in one call. </details> <details> <summary>Review outcome</summary> 1 [fix here] after promotion · 0 left open. Lint clean; unit suite 2081 pass, 0 fail, 2 skipped (local, macOS). - **Review:** completed, separate agent context, head `ee41b6d` / base `6d5f334`. One 🟡 finding, raised as follow-up and fixed here at the author's request: a Markdown-only stacked PR moved onto trunk by hand kept its stub passes, since neither workflow listened for `edited`. Also noted, left as is: the `changes` script is duplicated across the two workflows. - **Since review:** `ee41b6d → 251885f` re-reviewed by a separate agent context: finding resolved, no new findings. Checked title-only edits (earlier passes stand), base-move cancellation, the concurrency group expression for every event, and drafts. - **CodeRabbit:** not requested yet. </details> <details> <summary>Implementation notes</summary> - The file list comes from `gh api --paginate repos/…/pulls/N/files` (up to 3000 files), taking `previous_filename` too, so a `.js` renamed to `.md` counts as code. - The test is `grep -v '\.md$' > /dev/null`, not `grep -qv`. Not every `grep` handles `-q` with `-v` the same way. - An empty file list counts as code. - The `changes` job swaps `contents: read` for `pull-requests: read`; it never checks out. Fork PRs get that permission too. - The concurrency group gets an `-edit` suffix only for a title or body edit, so those runs cancel only each other. A base move joins the normal group and cancels the stale run. </details> Screenshots: nothing on screen changed. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Before each announcement, a screen reader reads a hidden line that
@wordpress/a11yadds to the page: "Notifications". The package writes it at DOM-ready, which is before the renderer has loaded its locale from main, so it stayed English in every language.What changes
Once
loadLocale()has applied the locale, it writes that line again with__('Notifications'), next to where it already sets the window's title. It uses the same msgid as@wordpress/a11yand as the notifications region's label, so all three share one translation.If the paragraph were ever created after the locale loads instead, a11y's own
__()would already be translated, and the null check skips the rewrite.How to test this
A journey covers it, and it's red without the change:
The first-run en-XA journey now checks that
#a11y-speak-intro-textreads[Ñóţíƒíçáţíóñš~~~~]. Without the fix it reads "Notifications".To look by hand (any platform, current head): run
npx electron . --lang=en-XAfrom the repository root and inspect#a11y-speak-intro-textin DevTools. It's bracketed and accented.What must not have happened: in English the line still reads "Notifications", and announcements (
speak()) still work. The full journey suite passes (147/147).Risks and limitations
None known. The paragraph lives outside
#root, so React never replaces it, and a11y'sspeak()andclear()only toggle itshiddenattribute.Related
Fixes #648. Part of #622.
Review outcome
0 [fix here] · 0 [follow-up].
.github/instructions/code-review.instructions.md. Reviewed head d6ce1eb, base 8303849 (trunk).npm run lintclean,npm test2078/2078, full journey suite 147/147.defer, sodomReadyruns a11y'ssetup()while it loads, before the locale arrives. The paragraph exists when it's rewritten.@wordpress/a11y,@wordpress/i18nand@wordpress/hooks, so a11y uses the app's i18n instance.'Notifications'with the toast region's label is correct.document.titleisn't a decision that belongs in a.cjsmodule.#root, which the scans don't look past.Nothing on screen changes, so there are no screenshots.
🤖 Generated with Claude Code