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: add a browser lib to produce sa11y minified JS bundle #32

Merged
merged 117 commits into from
Aug 19, 2020

Conversation

mohanraj-r
Copy link
Contributor

@mohanraj-r mohanraj-r commented Aug 12, 2020

  • Produce a Sa11y minified JS file sa11y.min.js that wraps over axe-core library
  • sa11y.min.js would be injected into the browser, from webdriver tests e.g., instead of axe-core JS library to provide the same set of Sa11y APIs across different testing workflows
  • sa11y.min.js would still expose the axe-core library same as axe.min.js so the existing APIs relying on axe.min.js would continue to work without having to rewrite everything to make use of the Sa11y APIs
  • A followup-PR will add functionality to add selected functionality from the Sa11y library (e.g. a11y results filtering) to the sa11y.min.js

TODO

Post merge

  • Publish github release
    • Skip npm release for this PR. Have a axe v4 upgrade PR that needs to be released next.

Mohan Raj Rajamanickam added 30 commits June 2, 2020 20:37
# Conflicts:
#	package.json
# Conflicts:
#	package.json
#	yarn.lock
@@ -0,0 +1,28 @@
# `@sa11y/browser-lib`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if browser-lib is an apt name for the library that produces sa11y.min.js.
Suggestions welcome!

@mohanraj-r mohanraj-r requested review from trevor-bliss and a team August 12, 2020 21:47
@mohanraj-r mohanraj-r marked this pull request as ready for review August 12, 2020 21:47
@mohanraj-r mohanraj-r self-assigned this Aug 13, 2020
@codecov
Copy link

codecov bot commented Aug 14, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@81c09be). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             master       #32   +/-   ##
==========================================
  Coverage          ?   100.00%           
==========================================
  Files             ?        14           
  Lines             ?       112           
  Branches          ?        11           
==========================================
  Hits              ?       112           
  Misses            ?         0           
  Partials          ?         0           


Provides a minified version of selected `@sa11y` libraries to be injected into a browser (using webdriver) and executed from integration testing workflows. This allows for reuse of the `@sa11y` libraries across unit and integration testing workflows.

Code in this package should be limited only to wrappers required to facilitate execution in browser environment. All primary code should be added to `@sa11y` libraries.

Choose a reason for hiding this comment

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

I'm a little unclear when you would use this package vs. the wdio package. I see the example usage below looks like it's even wdio sample code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is to be used in testing workflows without webdriverIO e.g. Java. Added clarification to doc. Please let me know if that answers your question.

Choose a reason for hiding this comment

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

Ok got it. That makes sense, but I still find it a little confusing that the sample below is in Javascript if the main target is Java. It'd be nice to see a more complete example authored in Java, even if the test cases in this project are still Javascript.

If I was a consumer trying to integrate this into my Java test suite, it would take some exploration still to figure out how exactly to use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, was thinking the same. Added Java example.

package.json Outdated
@@ -17,8 +17,9 @@
],
"scripts": {
"build": "tsc --build",
"build:ci": "yarn install --frozen-lockfile && yarn build",
"build:clean": "yarn build --clean && rimraf packages/**/dist && yarn install && yarn build",
"build:minjs": "yarn workspace @sa11y/browser-lib build",

Choose a reason for hiding this comment

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

I would expect this to run during the normal build, instead of only build:ci.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. Fixed. Was trying to hold on to tsc --build being used only once and reusing yarn build.

resolve(),
commonjs(),
typescript({ tsconfigOverride: { compilerOptions: { module: 'es2015' } } }),
terser(), // Note: Comment to get un-minified file for debugging etc

Choose a reason for hiding this comment

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

You could also produce an un-minified sa11y.js in addition to the minified sa11y.min.js. Then the consumer would choose which version they wanted to use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

export { sortViolations } from '@sa11y/format';
export { assertAccessible } from '@sa11y/assert';

export const nameSpace = 'sa11y';

Choose a reason for hiding this comment

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

This is usually "namespace" (all lowercase).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.


Provides a minified version of selected `@sa11y` libraries to be injected into a browser (using webdriver) and executed from integration testing workflows. This allows for reuse of the `@sa11y` libraries across unit and integration testing workflows.

Code in this package should be limited only to wrappers required to facilitate execution in browser environment. All primary code should be added to `@sa11y` libraries.

Choose a reason for hiding this comment

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

Ok got it. That makes sense, but I still find it a little confusing that the sample below is in Javascript if the main target is Java. It'd be nice to see a more complete example authored in Java, even if the test cases in this project are still Javascript.

If I was a consumer trying to integrate this into my Java test suite, it would take some exploration still to figure out how exactly to use it.

Copy link
Contributor Author

@mohanraj-r mohanraj-r left a comment

Choose a reason for hiding this comment

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

Thanks for the review @trevor-bliss
Addressed your comments. Please review.

@mohanraj-r mohanraj-r merged commit 8a44543 into salesforce:master Aug 19, 2020
@mohanraj-r mohanraj-r deleted the add_browser_lib branch August 19, 2020 20:04
@mohanraj-r mohanraj-r mentioned this pull request Aug 19, 2020
3 tasks
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.

2 participants