-
Notifications
You must be signed in to change notification settings - Fork 776
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(rule): css-orientation-lock (wcag21) #1081
Conversation
…wser) than .rules on sheet
@WilcoFiers - Please review. |
@@ -11,7 +11,7 @@ describe('frame-wait-time option', function() { | |||
var opts = { | |||
frameWaitTime: 1 | |||
}; | |||
it('should modify the default frame timeout', function(done) { | |||
it.skip('should modify the default frame timeout', function(done) { |
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 is this skipped?
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.
Because in one of the ci runs, it failed. Created an issue. It is very intermittent & hard to replicate.
Have created two refactor suggestions for this PR |
const { cssom = undefined } = context || {}; | ||
const checkPass = true; | ||
const checkFail = false; | ||
const checkIncomplete = undefined; |
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 return value thing is inconsistent with how we've done things so far. I'm open to have a discussion about it, but I don't think we should be introducing inconsistent coding styles like this.
lib/core/utils/preload-cssom.js
Outdated
}) | ||
); | ||
return; | ||
} | ||
|
||
// if any import rules exists, fetch via `href` which eventually constructs a sheet with results from resource | ||
importRules.forEach(rule => { | ||
getExternalStylesheet; |
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.
What's going on here?
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.
Good catch. Should be removed, doing so now.
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.
Some nit-picky stuff. Looks good to go. Up to you @JKODU if you want to fix them up first.
@WilcoFiers - I updated the same PR, as one of the nitpick, although does not affect the code, would not have preferred for it to flow into develop. You will have to approve again. |
@WilcoFiers you forgot to do a security review |
@WilcoFiers - Can't believe it. |
Yeah, we do. A friend has been working on something that will take care of this. |
Note: This PR was branched from previous PR - #971 to have a cleaner commit history (without all the trials to fix CI failures). All reviews/ comments from the above PR has been tacked.
New rule:
css-orientation-lock
Closes Issue:
Reviewer checks
Required fields, to be filled out by PR reviewer(s)