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

feat(configure): add axeVersion property that checks compatibility of axe.version #1793

Merged
merged 5 commits into from
Sep 12, 2019

Conversation

straker
Copy link
Contributor

@straker straker commented Aug 29, 2019

Closes issue: #1780

Reviewer checks

Required fields, to be filled out by PR reviewer(s)

  • Follows the commit message policy, appropriate for next version
  • Code is reviewed for security

@straker straker requested a review from a team as a code owner August 29, 2019 19:49
@straker straker changed the title feature(configure): add ver property that checks compatibility axe.version feature(configure): add ver property that checks compatibility of axe.version Aug 29, 2019
doc/API.md Outdated Show resolved Hide resolved
lib/core/public/configure.js Outdated Show resolved Hide resolved
test/core/public/configure.js Outdated Show resolved Hide resolved
@stephenmathieson
Copy link
Member

I think we should also update the TypeScript definition.

@straker
Copy link
Contributor Author

straker commented Aug 29, 2019

@WilcoFiers did you have any particular reason why you wanted the name of the prop to be ver instead of version?

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.

Please hold until we've got an opinion from @downzer0.

doc/API.md Outdated Show resolved Hide resolved
@stephenmathieson stephenmathieson changed the title feature(configure): add ver property that checks compatibility of axe.version feat(configure): add ver property that checks compatibility of axe.version Aug 30, 2019
@straker straker changed the title feat(configure): add ver property that checks compatibility of axe.version feat(configure): add axeVersion property that checks compatibility of axe.version Sep 3, 2019
lib/core/public/configure.js Outdated Show resolved Hide resolved
lib/core/public/configure.js Outdated Show resolved Hide resolved
@straker straker merged commit 18fb8c8 into develop Sep 12, 2019
@straker straker deleted the configureVersion branch September 12, 2019 17:03
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.

Couple of small points left.

@@ -8,6 +8,32 @@ function configureChecksRulesAndBranding(spec) {
throw new Error('No audit configured');
}

if (spec.axeVersion) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I should have caught this one earlier. But the ticket asks for adding both ver and axeVersion, where axeVersion is prioritised over ver if both are set. The intent is to have a fallback so that this can work for older custom rules as well.

(major === axeMajor &&
minor === axeMinor &&
patch === axePatch &&
((canary && axeCanary) || (canary && !axeCanary)))
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm thinking if the canary version is the same, it also shouldn't throw.

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.

3 participants