-
Notifications
You must be signed in to change notification settings - Fork 783
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
feat: add runtime localization support #1036
Changes from 1 commit
2dbcf8d
1067f4a
b5b64d8
8b2c704
a60a9ef
e036e85
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,8 +40,191 @@ function Audit(audit) { | |
|
||
this.defaultConfig = audit; | ||
this._init(); | ||
|
||
// A copy of the "default" locale. This will be set if the user | ||
// provides a new locale to `axe.configure()` and used to undo | ||
// changes in `axe.reset()`. | ||
this._previousLocale = null; | ||
} | ||
|
||
/** | ||
* Build and set the previous locale. Will noop if a previous | ||
* locale was already set, as we want the ability to "reset" | ||
* to the default ("first") configuration. | ||
*/ | ||
|
||
Audit.prototype._setPreviousLocale = function() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not a huge deal, but I'd prefer we use named functions where we can in axe-core. It tends to make debugging a lot easier. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you're mistaking this method for a function expression. Because this is a method, we'll see its implicit name in stack traces. Given the code: function F () {}
F.prototype.foo = function () {
throw new Error('boom')
}
const f = new F
f.foo() We'll clearly see "foo" in the stack trace:
|
||
if (this._previousLocale) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think instead of calling this previousLocale, we should call this defaultLocale or originalLocale. Same for the function names. We're not going back to the previous one. This is going back to the original locale (which is what reset should do). You may want to put a test in place for that as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 or fallbackLocale? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not a fallback, but instead, it's the default. I agree with Wilco that it should be called |
||
return; | ||
} | ||
|
||
const locale = { | ||
checks: {}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we keep track of a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not used by anything, so saving it didn't seem super important. What do you think we need it for? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a good point. I think we should include a lang key on the results object. I think we should do that separately from this PR though. |
||
rules: {} | ||
}; | ||
|
||
// XXX: unable to use `for-of` here, as doing so would | ||
// require us to polyfill `Symbol`. | ||
const checkIDs = Object.keys(this.data.checks); | ||
for (let i = 0; i < checkIDs.length; i++) { | ||
const id = checkIDs[i]; | ||
const check = this.data.checks[id]; | ||
const { pass, fail, incomplete } = check.messages; | ||
locale.checks[id] = { | ||
pass, | ||
fail, | ||
incomplete | ||
}; | ||
} | ||
|
||
const ruleIDs = Object.keys(this.data.rules); | ||
for (let i = 0; i < ruleIDs.length; i++) { | ||
const id = ruleIDs[i]; | ||
const rule = this.data.rules[id]; | ||
const { description, help } = rule; | ||
locale.rules[id] = { description, help }; | ||
} | ||
|
||
this._previousLocale = locale; | ||
}; | ||
|
||
/** | ||
* Reset the locale to the "default". | ||
*/ | ||
|
||
Audit.prototype._resetLocale = function() { | ||
// If no previous locale exists, then we haven't applied one | ||
// and the default is still in-tact. | ||
const previousLocale = this._previousLocale; | ||
if (!previousLocale) { | ||
return; | ||
} | ||
|
||
// Apply the previous locale | ||
this.applyLocale(previousLocale); | ||
}; | ||
|
||
/** | ||
* Merge two check locales (a, b), favoring `b`. | ||
* | ||
* Both locale `a` and the returned shape resemble: | ||
* | ||
* { | ||
* impact: string, | ||
* messages: { | ||
* pass: string | function, | ||
* fail: string | function, | ||
* incomplete: string | { | ||
* [key: string]: string | function | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think the value can be a function. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It can, and will be. The result of calling |
||
* } | ||
* } | ||
* } | ||
* | ||
* Locale `b` follows the `axe.CheckLocale` shape and resembles: | ||
* | ||
* { | ||
* pass: string, | ||
* fail: string, | ||
* incomplete: string | { [key: string]: string } | ||
* } | ||
*/ | ||
|
||
const mergeCheckLocale = (a, b) => { | ||
let { pass, fail } = b; | ||
// If the message(s) are Strings, they have not yet been run | ||
// thru doT (which will return a Function). | ||
if (typeof pass === 'string') { | ||
pass = axe.imports.doT.compile(pass); | ||
} | ||
if (typeof fail === 'string') { | ||
fail = axe.imports.doT.compile(fail); | ||
} | ||
return { | ||
impact: a.impact, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if they want to override the impact? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not currently supported with the existing localization setup. Adding it now falls out of scope of the task I was given. If it's something we know we want, we can add support after this PR lands. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 to Stephen's comment. You can change impact by doing this: axe.configure({
checks: [{
id:'foo',
metadata: { impact: 'minor' }
}]
}); I think that instead of copying over There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 I'll spread |
||
messages: { | ||
pass: pass || a.messages.pass, | ||
fail: fail || a.messages.fail, | ||
incomplete: | ||
typeof a.messages.incomplete === 'object' | ||
? // TODO: for compleness-sake, we should be running | ||
// incomplete messages thru doT as well. This was | ||
// out-of-scope for runtime localization, but should | ||
// eventually be addressed. | ||
{ ...a.messages.incomplete, ...b.incomplete } | ||
: b.incomplete | ||
} | ||
}; | ||
}; | ||
|
||
/** | ||
* Merge two rule locales (a, b), favoring `b`. | ||
*/ | ||
|
||
const mergeRuleLocale = (a, b) => { | ||
let { help, description } = b; | ||
// If the message(s) are Strings, they have not yet been run | ||
// thru doT (which will return a Function). | ||
if (typeof help === 'string') { | ||
help = axe.imports.doT.compile(help); | ||
} | ||
if (typeof description === 'string') { | ||
description = axe.imports.doT.compile(description); | ||
} | ||
return { | ||
help: help || a.help, | ||
description: description || a.description | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is missing the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good, will update! |
||
}; | ||
}; | ||
|
||
/** | ||
* Apply locale for the given `checks`. | ||
*/ | ||
|
||
Audit.prototype._applyCheckLocale = function(checks) { | ||
const keys = Object.keys(checks); | ||
for (let i = 0; i < keys.length; i++) { | ||
const id = keys[i]; | ||
if (!this.data.checks[id]) { | ||
throw new Error(`Locale provided for unknown check: "${id}"`); | ||
} | ||
|
||
this.data.checks[id] = mergeCheckLocale(this.data.checks[id], checks[id]); | ||
} | ||
}; | ||
|
||
/** | ||
* Apply locale for the given `rules`. | ||
*/ | ||
|
||
Audit.prototype._applyRuleLocale = function(rules) { | ||
const keys = Object.keys(rules); | ||
for (let i = 0; i < keys.length; i++) { | ||
const id = keys[i]; | ||
if (!this.data.rules[id]) { | ||
throw new Error(`Locale provided for unknown rule: "${id}"`); | ||
} | ||
this.data.rules[id] = mergeRuleLocale(this.data.rules[id], rules[id]); | ||
} | ||
}; | ||
|
||
/** | ||
* Apply the given `locale`. | ||
* | ||
* @param {axe.Locale} | ||
*/ | ||
|
||
Audit.prototype.applyLocale = function(locale) { | ||
this._setPreviousLocale(); | ||
|
||
if (locale.checks) { | ||
this._applyCheckLocale(locale.checks); | ||
} | ||
|
||
if (locale.rules) { | ||
this._applyRuleLocale(locale.rules); | ||
} | ||
}; | ||
|
||
/** | ||
* Initializes the rules and checks | ||
*/ | ||
|
@@ -367,4 +550,5 @@ Audit.prototype._constructHelpUrls = function(previous = null) { | |
Audit.prototype.resetRulesAndChecks = function() { | ||
'use strict'; | ||
this._init(); | ||
this._resetLocale(); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this enough for users to understand how to utilize the templating feature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not. I'm hoping to get some feedback here that helps me create the missing portion(s) of documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could add a note to the README: https://github.com/dequelabs/axe-core#localization
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@WilcoFiers and I have plans to work on documentation for this today.