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

A11y metatip color swatches (+ click to copy to clipboard) #505

Merged
merged 23 commits into from
May 14, 2021
Merged

A11y metatip color swatches (+ click to copy to clipboard) #505

merged 23 commits into from
May 14, 2021

Conversation

hchiam
Copy link
Contributor

@hchiam hchiam commented May 11, 2021

For #496
demo-1
As a simple fix to side-step contrast issues, I went with a text outline, instead of using mix-blend-mode: difference (which i think could end up with gray on gray). if you'd like, i can look further into how pika dynamically shows just black xor white text depending on the swatch color, which would require fancier js logic. Maybe i could also add some js logic to make the "copied!" message stay a bit longer. lemme know your thoughts! First time contributing to VisBug :)

Copy link
Collaborator

@argyleink argyleink left a comment

Choose a reason for hiding this comment

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

this is rad work! i def think we can get this merged without too much more effort. i really appreciate you hacking on this, it looks so much better already.

a minor thing is presenting the color based on the user preference. i've added some inline comments to help out.

the biggest thing to fix / think about / design (🙂 ), is when inspecting elements with aria roles and other a11y meta, the new color boxes shows their fixed size (see screenshot 1). to solve this and match pika, i think separating the score from this new visual would make the layout easier to handle being responsive. i think having your new color block side by side is best above the <code accessibility> element in ally.element.js.

image

Would be nice to have it be responsive too, so on mobile or testing mobile, the layout fits into a portrait layout

image

Is gitter or slack a better way to have some direct conversation? I want to help you and save you time by assisting you quickly 🙂 I wasnt able to quickly have a design solution locally, i think the templates need some attention. we're deviating from the metatip formula, so we'll need to teach it how to present these new elements! no prob, we can totally get there.

besides those 2 things, are you planning on matching Pika further by changing the way AA, AA+, AAA, AAA+ look? 😁

app/components/metatip/ally.element.js Outdated Show resolved Hide resolved
app/components/metatip/metatip.element.css Show resolved Hide resolved
app/components/metatip/metatip.element.css Show resolved Hide resolved
extension/manifest.json Outdated Show resolved Hide resolved
app/components/metatip/metatip.element.css Outdated Show resolved Hide resolved
app/features/accessibility.js Outdated Show resolved Hide resolved
app/features/accessibility.js Outdated Show resolved Hide resolved
@argyleink
Copy link
Collaborator

oh yeah! and tinycolor is available. you could use it's api to find out if white/black are better (so you dont need all the text shadows), here's the api https://www.npmjs.com/package/@ctrl/tinycolor. you might even be able to get more creative than than black/white, it's a powerful color tool 👍🏻

@hchiam
Copy link
Contributor Author

hchiam commented May 12, 2021

demo-2 demo-3

the other 2 things i'll need to look into are simply rearranging the AA, AA+, AAA, AAA+ section and hopefully fix the one failing test

hchiam and others added 7 commits May 12, 2021 17:30
navigator.permissions.query({name:'clipboard-write'}) currently won't work in firefox
fully address firefox 'clipboard-write' in #507

Co-authored-by: Adam Argyle <argyle@google.com>
Co-authored-by: Adam Argyle <argyle@google.com>
Copy link
Contributor Author

@hchiam hchiam left a comment

Choose a reason for hiding this comment

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

ready for one more check 😃 @argyleink #505 (comment)

@argyleink
Copy link
Collaborator

It's lookin so good! There's 2 remaining questions/tasks before this merges into main (which will cause a build and deploy to the app store!)

  1. ~match the presentation that pika has for the ratio scores. I put ~ in there because I don't think we need to 100% rip the design, like right now visbug is smart enough to choose between AA or AAA. We should keep that, but adopt nice parts of Pika's. thoughts?
  2. figure out how to show aria information and other accessibility meta data that visbug scrapes from elements. it should probably go under the contrast scores? it's an open design task.

you feeling up to these last items? you've done a great set of work here and I don't want to exhaust you! 🙂 how you feelin?

if it's helpful, i could create a design or 2 to show the direction i'm thinking (solve the design task), creating a focused engineering task?

Co-authored-by: Adam Argyle <argyle@google.com>
@hchiam
Copy link
Contributor Author

hchiam commented May 13, 2021

no worries, it's been fun! a design/mock would be helpful to make sure I'm on track. but yeah, feel free to make a separate branch that i can merge this PR into instead of main, for you to add to. meanwhile i'll think about the design and see how far i get

@hchiam
Copy link
Contributor Author

hchiam commented May 14, 2021

sizing+order+alignment

@argyleink argyleink changed the base branch from main to ally-metatip-v2 May 14, 2021 18:51
@argyleink
Copy link
Collaborator

you've done so much, it looks sooo good 🙂
when you're done, let me know and I'll merge this! I'll create a new PR once I've rounded out some of those remaining design items 👍🏻

I've updated this PRs base to merge into an "ally metatip v2" branch, so i/we can collaborate a bit easier and get it perfect before merging into main 💯

@hchiam
Copy link
Contributor Author

hchiam commented May 14, 2021

thanks! Sounds good, go ahead

@argyleink argyleink merged commit fa995ea into GoogleChromeLabs:ally-metatip-v2 May 14, 2021
argyleink added a commit that referenced this pull request May 18, 2021
* A11y metatip color swatches (+ click to copy to clipboard) (#505)

* added color swatches with contrast-safe text

* color swatch click event setup

* clipboard event (still a WIP)

* copy to clipboard actually works now

* tell user: copy/copied

* Apply some suggestions from code review

Co-authored-by: Adam Argyle <argyle@google.com>

* colormode and modemap setup

* more responsive. cleaner contrasting text with TinyColor.

* account for long aria attribute text affecting responsiveness

* further tweaks to responsively style A, AA, AAA, aria-label, etc

* one more flexbox tweak

* see if this fixes the test (updated HTML)

* revert contrastValueSelector. comment out first failing test.

let's see how it goes!

* clipboard.writeText works in firefox without this permission

navigator.permissions.query({name:'clipboard-write'}) currently won't work in firefox

* copyToClipboard: 'clipboard-write' method for now

fully address firefox 'clipboard-write' in #507

Co-authored-by: Adam Argyle <argyle@google.com>

* more stable getContrastingColor

Co-authored-by: Adam Argyle <argyle@google.com>

* ugly fix but gets rid of the empty div

* tabindex="0" -> focusable -> holds focus

* rid gap

* metatip.element.css color-swatch padding: 10px <- 0.5em

Co-authored-by: Adam Argyle <argyle@google.com>

* tweak the sizing and order

* alignment tweaks

* white-space:nowrap; removes need for 0.7em font-size i think

Co-authored-by: Adam Argyle <argyle@google.com>

* closer to matching target design

* fixes regression

* minor color updates

* remove commas from presentation teir

* fixes regression in no match state

* fixes spacing issue and remove emoji

* add space into copy prompt

* adds tag presentation style

* adds space into copied message too

* matches spacing of other elements

* spacing nit

Co-authored-by: Howard Chiam <hchiam@users.noreply.github.com>
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.

None yet

2 participants