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.

Sign up for GitHub

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

Jump to bottom

feat(selenium): Implement axe.runPartial support #61

Merged
merged 47 commits into from
Sep 26, 2022
Merged

feat(selenium): Implement axe.runPartial support #61

merged 47 commits into from
Sep 26, 2022

Conversation

AdnoC
Copy link
Contributor

@AdnoC AdnoC commented Sep 13, 2022

Details

By default now uses axe.runPartial and axe.finishRun to analyze the page rather than axe.run.

Also adds AxeBuilder.UseLegacyMode to switch back to using axe.run.

Closes Issue: #14
Closes Issue: #15

@CLAassistant
Copy link

CLAassistant commented Sep 13, 2022 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

dbjorge
dbjorge reviewed Sep 13, 2022
View reviewed changes
package.json Outdated Show resolved Hide resolved
dbjorge
dbjorge previously requested changes Sep 15, 2022
View reviewed changes
Copy link
Contributor

@dbjorge dbjorge left a comment

Choose a reason for hiding this comment

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

Thanks, reviewed the changes! I marked all the resolved comments as resolved - there were a couple still left unresolved and two new ones that I've left based on the new content since the last review.

dbjorge
dbjorge approved these changes Sep 21, 2022
View reviewed changes
Copy link
Contributor

@dbjorge dbjorge left a comment

Choose a reason for hiding this comment

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

Code looks good! It would still be nice to update the selenium package README's reference API section to include a mention of UseLegacyMode, similar to the @axe-core/playwright package.

straker
straker previously requested changes Sep 22, 2022
View reviewed changes
package.json
@@ -4,5 +4,5 @@
"version": "4.4.0",
"license": "MPL-2.0 AND MIT",
"repository": "dequelabs/axe-core-nuget",
"private": true
"private": true,
Copy link
Contributor

@straker straker Sep 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Incorrect JSON formatting.

Suggested change
"private": true,
"private": true

packages/selenium/test/RunPartial/UseLegacyTests.cs
InitDriver(browser);
GoToFixture("index.html");
#pragma warning disable CS0618
new AxeBuilder(WebDriver, CustomSource($"{axeSource}{axeForceLegacy}"))
Copy link
Contributor

Choose a reason for hiding this comment

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

This should use the legacy version of axe and not the one that forces it to be legacy.

@AdnoC AdnoC dismissed straker’s stale review September 23, 2022 11:25

Use real legacy axe-core

@AdnoC AdnoC merged commit 32cf0e9 into develop Sep 26, 2022
This was referenced Sep 28, 2022
Closed
Closed
@dbjorge dbjorge mentioned this pull request Oct 7, 2022
Merged
dbjorge added a commit that referenced this pull request Oct 7, 2022
@dbjorge
feat(selenium): fix default value and add API docs for new UseLegacyM…
94d1d9a 
…ode API (#68)

## Details
This PR adds API documentation for the new public `UseLegacyMode` API to the README docs for the selenium package. The docs are based on the similar docs for [`@axe-core/playwright`'s `setLegacyMode` API](https://github.com/dequelabs/axe-core-npm/tree/develop/packages/playwright#axebuildersetlegacymodelegacymode-boolean--true).

While I was writing the code sample for the docs, I noticed that the default value for the new API (previously `false`) made for the following confusing behavior:

```csharp
// Looks intuitively like it would be using legacy mode, but actually isn't
AxeResult axeResult = new AxeBuilder(webDriver)
    .UseLegacyMode()
    .Analyze();
```

This PR updates the default value on `UseLegacyMode`'s parameter to `true` to make this interaction less confusing and for consistency with the `@axe-core/playwright` behavior.

Follow-up to #61 / #15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dbjorge dbjorge dbjorge approved these changes

@straker straker straker left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@AdnoC @CLAassistant @dbjorge @straker