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

Allow transparency in color knob #2624

Merged
merged 5 commits into from
Jan 3, 2018
Merged

Conversation

Primigenus
Copy link
Contributor

Issue: #802

What I did

Instead of just passing color.hex, also pass the alpha value.

How to test

Add addon-knobs and use color() to enable the knob in the story, then try dragging the opacity slider in the color picker or manually setting an alpha value in the field labeled "A".

Re: automated testing, not sure. LMK!

Fixes storybookjs#802.

Instead of just passing `color.hex`, also pass the alpha value.
<SketchPicker
color={knob.value}
onChange={color =>
onChange(`
Copy link
Member

Choose a reason for hiding this comment

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

This code looks nice, but it generates a huge section of noise in the url:

Copy link
Member

Choose a reason for hiding this comment

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

Can you solve this somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is a formatting issue, since I used a template string and applied prettier to make sure it doesn't exceed the line length (which added newlines). What would you recommend in order to stay within storybook's style guide?

Copy link
Member

Choose a reason for hiding this comment

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

You can ignore the problematic eslint rule for this line.

Copy link
Member

Choose a reason for hiding this comment

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

@Primigenus that's weird, prettier shouldn't insert newlines into text, only into interpolations. And you actually don't have to run it by hand, it runs automatically as precommit hook

Copy link
Member

@ndelangen ndelangen left a comment

Choose a reason for hiding this comment

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

There a lot of noise in the url because of the template literal having whitespace

@ndelangen
Copy link
Member

Many thanks for the PR @Primigenus !

If think if you can remove all whitespace we're all good.

@codecov
Copy link

codecov bot commented Jan 3, 2018

Codecov Report

Merging #2624 into master will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2624      +/-   ##
==========================================
- Coverage   34.33%   34.33%   -0.01%     
==========================================
  Files         388      388              
  Lines        8681     8682       +1     
  Branches      884      906      +22     
==========================================
  Hits         2981     2981              
+ Misses       5100     5083      -17     
- Partials      600      618      +18
Impacted Files Coverage Δ
addons/knobs/src/components/types/Color.js 8.1% <0%> (-0.12%) ⬇️
app/vue/src/server/config/babel.js 0% <0%> (-100%) ⬇️
addons/knobs/src/angular/utils.js 0% <0%> (ø) ⬆️
addons/info/src/components/types/PropertyLabel.js 78.94% <0%> (ø) ⬆️
addons/info/src/components/types/Enum.js 0% <0%> (ø) ⬆️
lib/ui/src/modules/ui/configs/handle_keyevents.js 33.33% <0%> (ø) ⬆️
lib/ui/src/modules/ui/configs/init_panels.js 25% <0%> (ø) ⬆️
addons/jest/src/hoc/provideJestResult.js 0% <0%> (ø) ⬆️
...-native/src/preview/components/OnDeviceUI/index.js 0% <0%> (ø) ⬆️
... and 44 more

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 b4dec5f...5732bee. Read the comment docs.

@ndelangen ndelangen merged commit 16aa076 into storybookjs:master Jan 3, 2018
@ndelangen ndelangen changed the title Pass alpha transparency up to the story Allow transparency in color knob Jan 3, 2018
@ndelangen
Copy link
Member

Wonderful! Thanks! @Primigenus

@Primigenus Primigenus deleted the patch-2 branch January 3, 2018 23:24
@Hypnosphi Hypnosphi added the patch:yes Bugfix & documentation PR that need to be picked to main branch label Jan 3, 2018
shilman pushed a commit that referenced this pull request Jan 7, 2018
Pass alpha transparency up to the story
@Hypnosphi Hypnosphi added the patch:done Patch/release PRs already cherry-picked to main/release branch label Mar 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addon: knobs 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.

4 participants