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.

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

chore(text): update text content #4499

Merged
merged 6 commits into from
Jun 17, 2024

Conversation

marktnoonan
Copy link
Contributor

Hi! I noticed some minor typos and inconsistent text capitalization, as well as what looks like flipped help and description text in one spot for the landmark-unique rule. This PR fixes the ones I was able to spot.

I noticed some overlap with #4385, I'd be happy to grab the text changes mentioned there in this PR as well, ("Ensures" -> "Ensure" in rule descriptions), if that's not stepping on anybody's toes.

Fix minor issues like capitalization and spelling
@marktnoonan marktnoonan requested a review from a team as a code owner June 11, 2024 02:51
Copy link
Contributor

@WilcoFiers WilcoFiers left a comment

Choose a reason for hiding this comment

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

Thank you very much @marktnoonan. Would you mind making these changes in their source files instead of in the generated file though?

Sorry about that, we normally put a comment at the top of generated files. Can't put comments in JSON unfortunately.

Copy link
Contributor

Choose a reason for hiding this comment

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

@marktnoonan This is a generated file. You'll need to make these changes in in the different JSON files in lib/checks/*/{check-id}.json. This won't be used in axe-core, and it's going to get overwritten when this file is regenerated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh that makes so much sense, yes I'll move changes to the proper place, thanks!

@@ -86,9 +86,9 @@

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@WilcoFiers This file is also generated - should I remove these changes from the PR also?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, we try to keep this in sync with the .json files in the repo. We actually need the _template.json file updated too. Would you mind running npm run build to update that file as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah understood - we want the generated files updated and committed, but they are not edited directly, and so they stay current with the values used elsewhere in code. Updated the PR with the generated _template file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@WilcoFiers I believe everything is committed and good to go at this point.

Copy link
Contributor

@WilcoFiers WilcoFiers left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for the kind contribution @marktnoonan!

For the books; I reviewed this PR for security.

@WilcoFiers WilcoFiers merged commit 11fad59 into dequelabs:develop Jun 17, 2024
19 of 21 checks passed
@marktnoonan marktnoonan deleted the text-updates branch June 17, 2024 16:28
WilcoFiers added a commit that referenced this pull request Jul 29, 2024
##
[4.10.0](v4.9.1...v4.10.0)
(2024-07-29)

### Features

- **new-rule:** summary elements must have an accessible name
([#4511](#4511))
([0d8a99e](0d8a99e)),
closes [#4510](#4510)

### Bug Fixes

- **all-rules:** fix flakey all-rules firefox test
([#4467](#4467))
([3f13aa1](3f13aa1))
- **aria-allowed-attr:** allow aria-multiline=false for element with
contenteditable
([#4537](#4537))
([f019068](f019068))
- **aria-allowed-attr:** allow aria-required=false when normally not
allowed ([#4532](#4532))
([2e242e1](2e242e1))
- **aria-prohibited-attr:** allow aria-label/ledby on decendants of
widget ([#4541](#4541))
([07c5d91](07c5d91))
- **aria-roledescription:** keep disabled with { runOnly: 'wcag2a' }
([#4526](#4526))
([5b4cb9d](5b4cb9d)),
closes [#4523](#4523)
- **autocomplete-valid:** incomplete for invalid but safe values
([#4500](#4500))
([e31a974](e31a974)),
closes [#4492](#4492)
- **build:** limit locales to valid files when using the --all-lang
option ([#4486](#4486))
([d3db593](d3db593)),
closes [#4485](#4485)
- colorio.js patch mocking CSS
([#4456](#4456))
([3ef9353](3ef9353)),
closes [#4400](#4400)
- correct typos in texts
([#4499](#4499))
([11fad59](11fad59))
- **landmark-unique:** follow spec, aside -> landmark
([#4469](#4469))
([e32f803](e32f803)),
closes [#4460](#4460)
- **required-attr:** allow aria-valuetext on slider instead of valuenow
([#4518](#4518))
([135898b](135898b)),
closes [#4515](#4515)

This PR was opened by a robot 🤖 🎉
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants