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

[Lens] Add ability to set colors for y-axis series #70311

Merged
merged 20 commits into from
Jul 3, 2020
Merged

[Lens] Add ability to set colors for y-axis series #70311

merged 20 commits into from
Jul 3, 2020

Conversation

mbondyra
Copy link
Contributor

@mbondyra mbondyra commented Jun 30, 2020 •
edited
Loading

Summary

Fixes #53660

This PR adds ability to change the colors for Y-axis series.

The color stays default when it's not defined by user or when there's a breakdown by defined.

Checklist

  • check copy
  • [Unit or functional tests]
  • design for panel
  • to discuss the round robin color assignment
  • bug for colorPicker
  • accessibility?

@mbondyra mbondyra added Feature:Lens release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.9.0 v8.0.0 labels Jun 30, 2020
@cchaos
Copy link
Contributor

cchaos commented Jun 30, 2020

Here are some mocks to help you with those 3 different states.

  1. Auto/default color (we don’t know the hex value)
  2. Custom color, the user has decided to override the default color
  3. Auto/default color and user can’t change it because of a split-by

Screen Shot 2020-06-30 at 16 28 20 PM

The only thing crucially missing from the EUI side in the EuiColorPicker component is the ability to customize the placeholder text which displays as "Transparent" but we want "Auto". I'll get the EUI side rolling but maybe there's a workaround?

@mbondyra mbondyra marked this pull request as ready for review July 1, 2020 20:27
@mbondyra mbondyra requested a review from a team July 1, 2020 20:27
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@cchaos
Copy link
Contributor

cchaos commented Jul 1, 2020

Looks great to me, functionally. As FYI, I've created an issue for the placeholder and isClearable props on EuiColorPicker. elastic/eui#3683 We'll be sure it gets in for 7.9

cchaos
cchaos suggested changes Jul 1, 2020
View reviewed changes
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

It's also fun to see the suggestions update 😄

Here are some design suggestions to get the form to take up the full width.

x-pack/plugins/lens/public/xy_visualization/xy_config_panel.tsx Outdated Show resolved Hide resolved
x-pack/plugins/lens/public/xy_visualization/xy_config_panel.tsx Show resolved Hide resolved
x-pack/plugins/lens/public/xy_visualization/xy_config_panel.tsx Outdated Show resolved Hide resolved
x-pack/plugins/lens/public/xy_visualization/xy_config_panel.tsx Show resolved Hide resolved
x-pack/plugins/lens/public/xy_visualization/xy_config_panel.tsx Outdated Show resolved Hide resolved
x-pack/plugins/lens/public/xy_visualization/xy_config_panel.tsx Outdated Show resolved Hide resolved
flash1293
flash1293 approved these changes Jul 2, 2020
View reviewed changes
Copy link
Contributor

@flash1293 flash1293 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This looks great, thanks a lot @mbondyra ! Tested in Firefox and works as expected, LGTM. I left two small nits.

Side note: I noticed a bug when switching between xy chart types (e.g. bar -> line) when multiple axes are configured, but it's very possible this is not caused by this PR:

  • Create a chart with two axes
  • Switch between bar/line/area
  • Renders with an error
  • Change axis side back to auto, then pick the same side again, now it works

I will look into this separately

x-pack/plugins/lens/public/xy_visualization/xy_config_panel.tsx Outdated Show resolved Hide resolved
x-pack/plugins/lens/public/xy_visualization/xy_config_panel.tsx Outdated Show resolved Hide resolved
@mbondyra mbondyra changed the title [Lens] Set colors for y-axis series [Lens] Add ability to set colors for y-axis series Jul 2, 2020
@mbondyra mbondyra requested a review from cchaos July 2, 2020 17:51
cchaos
cchaos approved these changes Jul 2, 2020
View reviewed changes
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Looks great! I'll get the EUI portion followed up when we have it in Kibana. Just a couple suggestions to fix some layouts, but then it's gtg, from my side.

x-pack/plugins/lens/public/xy_visualization/xy_config_panel.tsx Show resolved Hide resolved
x-pack/plugins/lens/public/xy_visualization/xy_config_panel.tsx Outdated Show resolved Hide resolved
x-pack/plugins/lens/public/xy_visualization/xy_config_panel.tsx Outdated Show resolved Hide resolved
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@mbondyra mbondyra merged commit a916e0a into elastic:master Jul 3, 2020
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jul 3, 2020
@gmmorris
Merge branch 'master' into actions/feature
f56574f 
* master: (199 commits)
  [Telemetry] Add documentation about Application Usage (elastic#70624)
  [Ingest Manager] Improve agent unenrollment with unenroll action (elastic#70031)
  Handle timeouts on creating templates (elastic#70635)
  [Lens] Add ability to set colors for y-axis series (elastic#70311)
  [Uptime] Use elastic charts donut (elastic#70364)
  [Ingest Manager] Update registry URL to point to snapshot registry (elastic#70687)
  [Composable template] Create / Edit wizard (elastic#70220)
  [APM] Optimize services overview (elastic#69648)
  [Ingest Pipelines] Load from json (elastic#70297)
  [Rum Dashbaord] Rum selected service view (elastic#70579)
  [Uptime] Prevent duplicate requests on load for index status (elastic#70585)
  [ML] Changing shared module setup function parameters (elastic#70589)
  [Ingest Manager] Add ability to sort to agent configs and package configs (elastic#70676)
  [Alerting] document requirements for developing new action types (elastic#69164)
  Fixed adding an extra space character on selecting alert variable in action text fields (elastic#70028)
  [Maps] show vector tile labels on top (elastic#69444)
  chore(NA): upgrade to lodash@4 (elastic#69868)
  Add Snapshot Restore README with quick-testing steps. (elastic#70494)
  [EPM] Use higher priority than default templates (elastic#70640)
  [Maps] Fix cannot select Solid fill-color when removing fields (elastic#70621)
  ...
@flash1293 flash1293 mentioned this pull request Jul 3, 2020
Merged
flash1293 pushed a commit to flash1293/kibana that referenced this pull request Jul 3, 2020
@mbondyra @flash1293
[Lens] Add ability to set colors for y-axis series (elastic#70311)
55683eb
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jul 3, 2020
@gmmorris
Merge branch 'master' into alerting/consumer-based-rbac
c426139 
* master:
  [Lens] Fitting functions (elastic#69820)
  [Telemetry] Add documentation about Application Usage (elastic#70624)
  [Ingest Manager] Improve agent unenrollment with unenroll action (elastic#70031)
  Handle timeouts on creating templates (elastic#70635)
  [Lens] Add ability to set colors for y-axis series (elastic#70311)
  [Uptime] Use elastic charts donut (elastic#70364)
  [Ingest Manager] Update registry URL to point to snapshot registry (elastic#70687)
  [Composable template] Create / Edit wizard (elastic#70220)
  [APM] Optimize services overview (elastic#69648)
flash1293 added a commit that referenced this pull request Jul 3, 2020
@flash1293
[Lens] Add ability to set colors for y-axis series (#70311) (#70719)
2c0651e
@mbondyra mbondyra deleted the lens/axis-colors branch July 9, 2020 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flash1293 flash1293 flash1293 approved these changes

@cchaos cchaos cchaos approved these changes

@wylieconlon wylieconlon Awaiting requested review from wylieconlon

Assignees
No one assigned
Labels
Feature:Lens release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.9.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Lens] Set colors for y-axis series
5 participants
@mbondyra @cchaos @elasticmachine @kibanamachine @flash1293