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.

Sign up for GitHub

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

Jump to bottom

[canvas] Color fixes + Storybook 5 #34075

Merged
merged 10 commits into from
Apr 2, 2019
Merged

[canvas] Color fixes + Storybook 5 #34075

merged 10 commits into from
Apr 2, 2019

Conversation

clintandrewhall
Copy link
Contributor

Summary

(Fixes #33734 #33757 #33735)

This PR resolves issues with the mini color selector, the places the color picker was used, and removes complexity around color selection. It allows for CSS-based color strings while still ruling out duplicates in the color palette, and fixes bugs found recently.

This PR also includes + modifies #33896 and updates Storybook to v5.

Mar-28-2019 09-18-45

@clintandrewhall clintandrewhall requested review from a team as code owners March 28, 2019 15:02
@clintandrewhall clintandrewhall added review Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.0.0 v7.2.0 labels Mar 28, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-canvas

@elasticmachine
Copy link
Contributor

💔 Build Failed

@ryankeairns
Copy link
Contributor

ryankeairns commented Mar 28, 2019 •
edited
Loading

Looks good, just a couple of suggestions with regards to the help and placeholder text.
To make it more discoverable, it would be good to note the options as follow:

Change the helpText prop value to...

Screenshot 2019-03-28 12 52 00

^ technically, 'HTML color names' is most correct, but for consistency with the placeholder text (below), this could alternatively say 'HTML color codes'

Change the placeholder text to then be more generic..

Screenshot 2019-03-28 12 52 37

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@snide
Copy link
Contributor

snide commented Mar 28, 2019 •
edited
Loading

Please test this in IE. Last time a storybook PR went in, it broke IE for all of Kibana in master.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@clintandrewhall
Copy link
Contributor Author

@snide Which PR broke IE, and how?

@snide
Copy link
Contributor

snide commented Mar 28, 2019

@clintandrewhall This was the fix we added #32159 after storybook was initially added.

Just be careful with such a large dependency in Kibana. So if you move it around or update it in anyway, please check it out in various browsers since jenkins doesn't check this kind of stuff.

@clintandrewhall
Copy link
Contributor Author

@snide good to know, I appreciate the heads up on this. I figured my changes were safe because it was an x-pack dev dependency, but I can see where the problem came from. I tested my component in IE11, but I'll double-check Kibana as well.

@clintandrewhall
Copy link
Contributor Author

@snide Just checked: it loads and functions... let me know if there's anything specific you'd be concerned about... from the look of the last time this failed, I should see something nasty either starting or loading Kibana... but I'll ask all the same.

Jenkins, test this.

@snide
Copy link
Contributor

snide commented Mar 28, 2019

Nope. No worries. It's new too so breaks happen. Just want to make sure you check it out if you ever do a version bump like this one on it.

w33ble
w33ble approved these changes Mar 28, 2019
View reviewed changes
Copy link
Contributor

@w33ble w33ble left a comment

Choose a reason for hiding this comment

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

Only looking at the color picker changes, they look good and do in fact fix the listed issues.

Not mixing the Storybook stuff with the fixes would have been nice.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

snide
snide approved these changes Apr 1, 2019
View reviewed changes
Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

Only checked the styling code. You can make some optimizations if you're trying to stay on point, but the sass changes look OK.

x-pack/plugins/canvas/public/components/color_picker_popover/color_picker_popover.scss Outdated
@@ -0,0 +1,3 @@
.canvasColorPickerPopover__popover {
width: 250px;
Copy link
Contributor

Choose a reason for hiding this comment

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

Generally we try to avoid pixel values so that we know the designs are based off the EUI math and not brittle. Something like $euiSize * 15.

...canvas/public/components/color_picker_popover/__examples__/color_picker_popover.examples.tsx Show resolved Hide resolved
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@clintandrewhall
Copy link
Contributor Author

clintandrewhall commented Apr 2, 2019 •
edited
Loading

@ryankeairns I updated the help text. Let me know if this doesn't match your expectation.

Screen Shot 2019-04-01 at 10 25 49 PM
Screen Shot 2019-04-01 at 10 25 55 PM
Screen Shot 2019-04-01 at 10 26 51 PM

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@clintandrewhall clintandrewhall merged commit 365fd4b into elastic:master Apr 2, 2019
clintandrewhall added a commit to clintandrewhall/kibana that referenced this pull request Apr 2, 2019
@clintandrewhall
[canvas] Color fixes + Storybook 5 (elastic#34075)
8e63d9f 
(Fixes elastic#33734 elastic#33757 elastic#33735)

This PR resolves issues with the `mini` color selector, the places the color picker was used, and removes complexity around color selection.  It allows for CSS-based color strings while still ruling out duplicates in the color palette, and fixes bugs found recently.

This PR also includes + modifies elastic#33896 and updates Storybook to v5.

![Mar-28-2019 09-18-45](https://user-images.githubusercontent.com/297604/55168267-873a1c00-5140-11e9-93b9-f5ca86e70098.gif)
@clintandrewhall clintandrewhall mentioned this pull request Apr 2, 2019
Merged
clintandrewhall added a commit that referenced this pull request Apr 2, 2019
@clintandrewhall
[7.x] [canvas] Color fixes + Storybook 5 (#34075) (#34369)
d4a8454 
Backports the following commits to 7.x:
 - [canvas] Color fixes + Storybook 5  (#34075)
@clintandrewhall clintandrewhall deleted the color branch June 6, 2019 04:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@w33ble w33ble w33ble approved these changes

@snide snide snide approved these changes

@cqliu1 cqliu1 Awaiting requested review from cqliu1

@rashidkpc rashidkpc Awaiting requested review from rashidkpc

Assignees
No one assigned
Labels
review Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v7.2.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Invalid Canvas page background breaks color selection
5 participants
@clintandrewhall @elasticmachine @ryankeairns @snide @w33ble