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

Fixed Colorblindness Emulation #6297

Merged
merged 1 commit into from
Mar 26, 2019
Merged

Fixed Colorblindness Emulation #6297

merged 1 commit into from
Mar 26, 2019

Conversation

electricg
Copy link
Contributor

Issue: #5959

What I did

How to test

Run storybook in Firefox and use the Colorblindness selection list

@codecov
Copy link

codecov bot commented Mar 26, 2019

Codecov Report

Merging #6297 into next will decrease coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next    #6297      +/-   ##
==========================================
- Coverage   38.29%   38.27%   -0.02%     
==========================================
  Files         648      648              
  Lines        9839     9844       +5     
  Branches      387      387              
==========================================
  Hits         3768     3768              
- Misses       6011     6016       +5     
  Partials       60       60
Impacted Files Coverage Δ
addons/a11y/src/register.tsx 0% <ø> (ø) ⬆️
addons/a11y/src/components/ColorBlindness.tsx 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7a9e145...ff08dd0. Read the comment docs.

@Armanio
Copy link
Member

Armanio commented Mar 26, 2019

Thank you!
I notice it bug, but forgot to fix it.
One moment: could you update snapahots?

@Armanio Armanio self-assigned this Mar 26, 2019
@electricg
Copy link
Contributor Author

Thank you!
I notice it bug, but forgot to fix it.
One moment: could you update snapahots?

Which snapshots? The only test I see for this addon is for the A11YPanel that I did not changed.

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.

Thanks for fixing this! Minor comment above

@@ -8,6 +8,16 @@ import { Icons, IconButton, WithTooltip, TooltipLinkList } from '@storybook/comp

const getIframe = memoize(1)(() => document.getElementById('storybook-preview-iframe'));

const getFilter = (filter: string | null) => {
if (filter === null) {
Copy link
Member

Choose a reason for hiding this comment

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

is !filter better?

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 could, but I wanted to be "strict" and consistent, since the reset is this.setFilter(null); and there's a similar check if (filter !== null) {

@shilman shilman added patch:yes Bugfix & documentation PR that need to be picked to main branch and removed ui labels Mar 26, 2019
@shilman shilman added this to the 5.0.x milestone Mar 26, 2019
@Armanio
Copy link
Member

Armanio commented Mar 26, 2019

Oh, of course. TeamCity build confused me.
Looks great! Wait another opinion.

@shilman shilman merged commit ab8f948 into storybookjs:next Mar 26, 2019
@shilman shilman added the patch:done Patch/release PRs already cherry-picked to main/release branch label Apr 1, 2019
shilman added a commit that referenced this pull request Apr 1, 2019
Fixed Colorblindness Emulation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addon: a11y bug patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants