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

Get proper contrast when hovering over svg #388

Merged
merged 7 commits into from Oct 4, 2019

Conversation

@adhrinae
Copy link
Contributor

commented Sep 22, 2019

Resolve #384

  • For the node which has fill or stroke (possibly path tag) attribute, use that value first.
  • Otherwise use color attribute instead.
  • If the mouseover target node is SVG, hide a11y tooltip.

I assumed calling getStyle function 3 times might be better than calling getStyles and checking attributes(fill, stroke, color) at once for performance perspective.

Since SVG node doesn't have any attribute related to the color, I think an a11y tooltip for svg doesn't have to appear.

@argyleink

This comment has been minimized.

Copy link
Collaborator

commented Sep 24, 2019

sick work, here's your PR deployed to glitch for testing/QA
https://svg-contrast.glitch.me

also, made a quick gif of me using it

svg-contrast

Works great! I'll do a code review here soon

app/features/accessibility.js Outdated Show resolved Hide resolved
Copy link
Collaborator

left a comment

❤️ 👍

@argyleink

This comment has been minimized.

Copy link
Collaborator

commented Sep 25, 2019

before i accept and merge, do you have interest in writing tests for the feature you implemented?

@adhrinae

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2019

Sure! I'll try for that.

@argyleink

This comment has been minimized.

Copy link
Collaborator

commented Sep 26, 2019

cool!

here's a few resources to help ya (and you can always ping me too)

@adhrinae

This comment has been minimized.

Copy link
Contributor Author

commented Sep 27, 2019

I've written some tests focusing on color contrast values.
Perhaps I can write more tests for the a11y tool later, like checking aria-* properties, etc.

Copy link
Collaborator

left a comment

you crushed it with these tests, thank you so much. you followed the previous patterns, you introduced as little as possible while still fully covering your new feature, and you were swift. LOVE IT! did i say i love it yet?

💯

that being said:

i'd love to hear how onboarding was for you:

  • Wiki helpful?
  • Any issues running tests locally?
  • Gotcha's that you would like added to the wiki for future folks?

it does look like 2 tests are failing, i've pointed out where i think the problem is in this review set. do you have ideas/thoughts on why those 2 tests using page.hover might fail in the cloud but not on your local machine?

app/features/accessibility.test.js Outdated Show resolved Hide resolved
app/features/accessibility.test.js Show resolved Hide resolved
app/features/accessibility.test.js Outdated Show resolved Hide resolved
@adhrinae

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2019

About my testing challenge experience

  • Wiki was just a little bit helpful. only for npm commands. I already knew how good ava & puppeteer are.
  • I have an experience writing UI tests (not e2e tho), however, to a person who has never written tests It is quite hard to write tests only referencing this wiki.
  • During testing, My main issue was the testing environment. It's a headless chrome so that I couldn't see how things are going on. as I mentioned the comment by LOC.
  • But puppeteer offers enough amounts of APIs for testing. so I had fun with reading API docs and trying them.
  • I recommend to list up 'good testing tips' in the wiki. like-
    • links to ava & puppeteer API docs
    • commonly used helper functions in tests
    • how to grab the DOM node I want exactly
    • escape hatch (visual testing, like the script I wrote above)
@adhrinae

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2019

@argyleink hey 👋 I understand you've been busy, so here's a gentle reminder for this PR. Please let me know if there's anything I should do more.

@argyleink

This comment has been minimized.

Copy link
Collaborator

commented Oct 4, 2019

yes, thanks for the nudge, you rock!

@argyleink argyleink merged commit 60c5ab5 into GoogleChromeLabs:master Oct 4, 2019
2 checks passed
2 checks passed
cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@adhrinae adhrinae deleted the adhrinae:fix/svg-accessibility branch Oct 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.