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

Feature/highlight a11y violation on component #6218

Conversation

CodeByAlex
Copy link
Member

@CodeByAlex CodeByAlex commented Mar 21, 2019

Issue: #6001

What I did

I added highlight toggles for each accessibility rule and category at every level. A user can now click on the highlight checkbox and see their violation, pass, or incomplete rule highlighted in the iframe. Each type (violation, pass, or incomplete) has a corresponding color to differentiate the rule when highlighting.

Design:

Highlight-Toggle component - can take in one to many elements that it can highlight. This means that the toggle component can be placed at any level and highlight the element or elements passed in based on a switch.

Redux is being used to keep track of a global map where the key is the element itself and the value is an object containing the toggle state of the element and the original element outline so that the component isn't changed when the highlight is removed.

The highlight toggle uses the redux map to keep track of whether or not the element or elements within it are highlighted and whether or not the toggle state is true.

I worked with @domyen on the design elements for this feature to keep consistency across the app.

How to test

  • Is this testable with Jest or Chromatic screenshots? yes
  • Does this need a new example in the kitchen sink apps? no
  • Does this need an update to the documentation? Maybe to describe the feature?

a11y-highlight-feature

CodeByAlex and others added 24 commits March 14, 2019 06:04
Merging Storybook Next into fork
# Conflicts:
#	addons/a11y/src/components/A11YPanel.tsx
…gles are set to true a user should be able to set the parent toggle to true and make all subs true (with intervention)
…://github.com/CodeByAlex/storybook into feature/highlight-a11y-violation-on-component

# Conflicts:
#	yarn.lock
…eature/highlight-a11y-violation-on-component-merge

# Conflicts:
#	addons/a11y/package.json
#	addons/a11y/src/components/A11YPanel.tsx
#	addons/a11y/src/components/Report/index.tsx
@CodeByAlex
Copy link
Member Author

CodeByAlex commented Mar 24, 2019

Thanks @domyen! Thanks for the feedback. I'll make sure that the "Highlight results" and checkbox are vertically centered and uncapitalize "results". My only concern about aligning the checkboxes to the right would be in scenario where someone uses a wide screen. It may be hard to determine which checkbox is associated with which rule (one above or below in most cases). What do you think?

@domyen
Copy link
Member

domyen commented Mar 24, 2019

👍 We can solve this by showing the boundaries of the row:
image

The right hand alignment makes the highlight checkbox a bit less distracting. After all the goal is to communicate the rules not that there's a checkbox. :)

And while we're here:

  • The button should extend to the full length of the row
  • The button should have a cursor:pointer
  • button:hover should have bg color rgba(0,0,0,.05)

You're a legend @CodeByAlex 🙏 🙇

One more thing

  • the border bottom color var is theme.color.border

@CodeByAlex
Copy link
Member Author

CodeByAlex commented Mar 24, 2019

I like that a lot @domyen, it looks a lot cleaner. Ill make that design change

@CodeByAlex
Copy link
Member Author

@domyen, Ive made the changes. Let me know what you think
a11y-highlight-feature-mod

…igned checkbox to the right without visible label (aria-label used)
@CodeByAlex CodeByAlex self-assigned this Mar 24, 2019
@CodeByAlex
Copy link
Member Author

CodeByAlex commented Mar 24, 2019

I think one other touch-up that would make this look a little sleeker would be to put the "Highlight results" text on the left side of the checkbox. This keeps a uniform line down the right side. I added that change in the latest commit.

image

image

@CodeByAlex CodeByAlex requested a review from domyen March 24, 2019 18:25
Copy link
Member

@domyen domyen left a comment

Choose a reason for hiding this comment

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

LGTM from a UI perspective

@CodeByAlex CodeByAlex requested a review from Armanio March 25, 2019 00:02
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

This looks amazing! Great work @CodeByAlex and @domyen! Esp love the right aligned checkbox on top -- details ❤️❤️❤️ 👌 !

Since the UI has gotten complex, and it's been awhile since I used redux for state management (cc @alexandrebodin @ndelangen), here's what I'd like to see on the code side:

  • Stories for the major components
  • Hooked into official-storybook or another top-level storybook to simplify testing

This might be outside the scope of this PR, but we should probably do this for all the core addons. It bothers me to see Jest tests with snapshots when we've already built a much nicer way to do this inside Storybook.

@shilman shilman added this to the v5.1.0 milestone Mar 25, 2019
@CodeByAlex CodeByAlex requested a review from ndelangen March 25, 2019 16:41
@ndelangen ndelangen self-assigned this Mar 25, 2019
# Conflicts:
#	addons/a11y/package.json
#	addons/a11y/src/components/__snapshots__/A11YPanel.test.js.snap
#	yarn.lock
@ndelangen
Copy link
Member

@CodeByAlex can you please allow maintainers to push to your branch? I'd like to fix the merge conflicts and merge

@CodeByAlex
Copy link
Member Author

CodeByAlex commented Mar 25, 2019

Sure thing @ndelangen. I checked the "Allow edits from maintainers option" so you should be good now

@ndelangen
Copy link
Member

Thank you! Let's get this merged, amazing feature!

@CodeByAlex
Copy link
Member Author

CodeByAlex commented Mar 25, 2019

Thanks, @ndelangen! Glad you like it

@Armanio
Copy link
Member

Armanio commented Mar 25, 2019

Looking great!
I wrote a few comments. Check it before you merge it, please.
Thank you for your effort.

@ndelangen ndelangen merged commit 3946fde into storybookjs:next Mar 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants