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

i18n: convert accessibility audits #6229

Merged
merged 3 commits into from
Nov 6, 2018
Merged

i18n: convert accessibility audits #6229

merged 3 commits into from
Nov 6, 2018

Conversation

patrickhulce
Copy link
Collaborator

Summary
converts the non-manual accessibility audits to use i18n strings

Copy link
Member

@exterkamp exterkamp left a comment

Choose a reason for hiding this comment

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

More i18n 🎉 Love it!

A few nits on wording.

Also I didn't leave a specific comment for the comments explaining description's but is it okay that they are all the same? The comment is a good one, but I don't know if it works in all cases? Does it need more context?

title: '`[aria-*]` attributes match their roles',
/** Title of an accesibility audit that evaluates if the aria HTML attributes are misaligned with the aria-role HTML attribute specificed on the element, such mismatches are invalid. This imperative title is shown to users when there is a failure that needs to be addressed. */
failureTitle: '`[aria-*]` attributes do not match their roles',
/** Description of a Lighthouse audit that tells the user *why* they should try to pass. This is displayed after a user expands the section to see more. No character length limits. 'Learn More' becomes link text to additional documentation. */
Copy link
Member

Choose a reason for hiding this comment

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

Need more details calling out what the ARIA attributes are? Might be confusing.

Note: This happens a lot, and I wonder if just saying ARIA, or aria-elements is sufficient in all places with no context?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe we can add it to our separate definitions glossary? @paulirish @brendankenny that was a thing right I'm not making that up :)

lighthouse-core/audits/accessibility/axe-audit.js Outdated Show resolved Hide resolved
lighthouse-core/audits/accessibility/meta-refresh.js Outdated Show resolved Hide resolved
@@ -31,6 +31,38 @@ const UIStrings = {
diagnosticsGroupTitle: 'Diagnostics',
/** Description of the diagnostics section of the Performance category. Within this section are audits with non-imperative titles that provide more detail on the page's page load performance characteristics. Whereas the 'Opportunities' suggest an action along with expected time savings, diagnostics do not. Within this section, the user may read the details and deduce additional actions they could take. */
diagnosticsGroupDescription: 'More information about the performance of your application.',
/* Title of the color contrast section within the Accessibility category. Within this section are audits with descriptive titles that highlight the color and vision aspects of the page's accessibility that are passing or failing. */
a11yColorContrastGroupTitle: 'Color Contrast Is Satisfactory',
Copy link
Member

Choose a reason for hiding this comment

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

'Color Contrast Is is Satisfactory'?

This happens a few times, do we mean to capitalize these words every time, or do we need to look at them to make sure they are following a capitalization guideline?

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've got no dog in this fight just moved the string up :)

I think the thinking here was that we have very few such small non-title-cased words that it just stood out too much, but 🤷‍♂️

lighthouse-core/config/default-config.js Outdated Show resolved Hide resolved
lighthouse-core/config/default-config.js Outdated Show resolved Hide resolved
lighthouse-core/config/default-config.js Outdated Show resolved Hide resolved
@patrickhulce
Copy link
Collaborator Author

Thanks for the thorough review @exterkamp!!

@patrickhulce
Copy link
Collaborator Author

Anyone else care about these?

@exterkamp
Copy link
Member

exterkamp commented Nov 5, 2018

@paulirish @brendankenny Can we resolve the conflicts then merge these in? If we roll these changes + the new i18n error changes into 1 translation request it'd be less rolling.

@patrickhulce
Copy link
Collaborator Author

conflicts fixed! 🎉 is this happening!? :D

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

I didn't review closely but I want to get on the hype train too 🚂🚂

/* Title of the meta tag section within the Accessibility category. Within this section are audits with descriptive titles that highlight if meta tags on the page have been used properly and if any important ones are missing. */
a11yMetaGroupTitle: 'Meta Tags Used Properly',
/* Description of the meta tag section within the Accessibility category. Within this section are audits with descriptive titles that highlight if meta tags on the page have been used properly and if any important ones are missing. */
a11yMetaGroupDescription: 'These are opportunities to improve the user experience of your site.',
Copy link
Member

Choose a reason for hiding this comment

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

lol this is going to dwarf the rest of the config soon

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.

None yet

4 participants