Fix all 7 ESLint errors reported by npm run lint - #770
Conversation
- ableplayer-base.js: the descriptionsAudible else-if duplicated the first condition (no-dupe-else-if), so the documented singular spelling data-description-audible was never honored. Read the singular attribute (options.descriptionAudible ?? data.descriptionAudible) in the second branch, matching the existing comment's intent. - dialog.js: import AblePlayer for the AblePlayer.getActiveDOMElement() call (no-undef). - dragdrop.js: remove unused thisObj locals in handleWindowButtonClick and handleMenuChoice (no-unused-vars); document the intentionally-empty Space/Enter branch (no-empty). - preference.js: remove the write-only $thisLabel variable and a dead $thisField assignment (no-unused-vars, no-useless-assignment). npm run lint: 0 problems. jsdom test suites: 55/55 pass. Build artifacts intentionally omitted per contributing.md. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
joedolson
left a comment
joedolson
left a comment
There was a problem hiding this comment.
Thanks for the contribution! One change requested - and it's on the line you'd already tagged as odd, so I don't imagine you're surprised.
| if (descriptionsAudible !== undefined && descriptionsAudible === false) { | ||
| this.readDescriptionsAloud = false; | ||
| } else if (descriptionsAudible !== undefined && descriptionsAudible === false) { | ||
| } else if (descriptionAudible !== undefined && descriptionAudible === false) { |
There was a problem hiding this comment.
This took some research. I traced back the origin of this, and learned that data-description-audible was added as an alternate code path for turning this on in the original commit for the feature. However, it's never been in the documentation - it seems like this was just a protection against the possibility of spelling the attribute differently.
Based on that, I think that it would be preferable to eliminate the alternate condition. It's technically a breaking change, but I don't personally see a lot of value to protecting incorrect attribute name paths unless they were at some point changed or documented differently than the current standard - and that isn't the case here.
Per review: the singular `data-description-audible` alternate code path was added in the feature's original commit as protection against misspelling the attribute, but was never documented and never the standard form. Eliminate the alternate condition rather than keep a lint-clean version of it; `data-descriptions-audible` (plural) is the one supported spelling.
|
Agreed — removed the alternate condition entirely rather than keeping a lint-clean version of an undocumented path. |
…#773 review fixes into develop # Conflicts: # .github/workflows/ci.yml # e2e/a11y.spec.js # e2e/keyboard.spec.js # e2e/serve.mjs # jest-puppeteer.config.cjs # playwright.config.js
2 participants
Hi @joedolson — we run Able Player v5 in production (YouTube grids on an accessibility-first creator platform) and would like to start contributing back. Starting small with the lint baseline so later PRs stay clean.
What
Fixes every error
npm run lintcurrently reports ondevelop(7 errors, 4 files):descriptionsAudibleelse-if repeats the first condition (no-dupe-else-if). The comment says the branch supports "both singular and plural spelling of attribute", but the singular form was never read. The branch now readsoptions.descriptionAudible ?? data.descriptionAudible, sodata-description-audibleworks as the comment intends. Happy to drop this hunk and simply delete the dead branch if the singular spelling was never meant to be supported.AblePlayerimport forAblePlayer.getActiveDOMElement()(no-undef).thisObjlocals (no-unused-vars); comments the intentionally-empty Space/Enter branch (no-empty).$thisLabeland one dead$thisFieldassignment (no-unused-vars,no-useless-assignment).Verification
npm run lint: 0 problems (was 7)npx jest --selectProjects jsdom: 55/55 passnpm run build: clean; build artifacts not committed per contributing.md🤖 Generated with Claude Code