-
Notifications
You must be signed in to change notification settings - Fork 791
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: deprecate the use doT.js for messages #1938
Conversation
@@ -3,7 +3,9 @@ let nodeName = node.nodeName.toUpperCase(); | |||
let type = (node.getAttribute('type') || '').toLowerCase(); | |||
let label = node.getAttribute('value'); | |||
|
|||
this.data(label); | |||
if (label) { |
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.
The message for this check used the existence of a label to determine the output, which doesn't work with the current schema. So I updated it since the data only needed to know a label was present and not what it was.
@@ -5,7 +5,10 @@ | |||
"impact": "critical", | |||
"messages": { | |||
"pass": "ARIA attributes are used correctly for the defined role", | |||
"fail": "ARIA attribute{{=it.data && it.data.length > 1 ? 's are' : ' is'}} not allowed:{{~it.data:value}} {{=value}}{{~}}" | |||
"fail": { | |||
"singular": "ARIA attribute is not allowed: ${data.values}", |
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.
This is 1,000,000x easier to read/follow ❤️
var templates = require('./templates'); | ||
var buildManual = require('./build-manual'); | ||
var entities = new (require('html-entities')).AllHtmlEntities(); | ||
var dotRegex = /\{\{.+?\}\}/g; |
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.
Don't think I'm familiar with a .+?
in regex, what does that do?
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.
The ?
makes the .+
not greedy. So instead of {{foo bar}} {{foo}}
matching the entire string, it will stop at the first set of }}
so you get 2 matches. Not that it really matters in this case but I'm use to making things not greedy.
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.
ahh, didn't know that. Cool
test/integration/full/async/async.js
Outdated
pass: 'function (out) { return "passed with " + out.data }', | ||
fail: 'function (out) { return "failed with " + out.data }', | ||
incomplete: | ||
'function (out) { return "incomplete with " + out.data }' |
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.
Why did you revert this?
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.
To show that the code still works with backwards compatibility
Removes doT.js from processing our check messages, allowing us to run translated messages on sites with a strict CSP. Doing so required a change in structure to some of our messages:
The changes are backward compatible with the old doT style template strings. The configure and build code looks at each string to see if it includes doT syntax, and if it does then runs it through the doT template. Otherwise it just leaves it as a string (the new style format). This means messages can be either a doT function or a string, which the code will handle appropriately.
Please manually test that:
axe.configure()
with a non-en locale worksgrunt build --lang=<langcode>
with a non-en locale worksgrunt translate --lang=<langcode>
output uses the newer structureReviewer checks
Required fields, to be filled out by PR reviewer(s)