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

[XY axis] Fixes the values inside bar charts #106198

Merged
merged 3 commits into from
Jul 21, 2021
Merged

[XY axis] Fixes the values inside bar charts #106198

merged 3 commits into from
Jul 21, 2021

Conversation

stratoula
Copy link
Contributor

@stratoula stratoula commented Jul 20, 2021 •
edited
Loading

Summary

Part of #101573.

The values inside the bars for our new XY axis plugin were not depicted always in the best way. I did the following changes:

  • The text color changes automatically depending on the text contrast
  • I have removed the white outline (and not give the option to the user). I think that by enabling the above it is is not needed
  • I have used a smaller font size
  • I display the label always in the center of the bar
  • Thanx to the new elastic-charts release, the values are not depicted if they do not fit.

Screenshots:
Screenshot 2021-07-20 at 10 30 29 AM
Screenshot 2021-07-20 at 10 30 15 AM
Screenshot 2021-07-20 at 10 30 02 AM
Screenshot 2021-07-20 at 10 29 46 AM
image

@stratoula
Copy link
Contributor Author

stratoula commented Jul 20, 2021 •
edited
Loading

@markov00 @ghudgins this is my proposal for XY axis. Wdyt?
The most important change is the white outline. I have removed it. I think that if we use the textContrast configuration the values are readable and there is no need to also give the ability to the users to add the border.

Does it also make sense for Lens? :)

@markov00
Copy link
Member

@markov00 @ghudgins this is my proposal for XY axis. Wdyt?
The most important change is the white outline. I have removed it. I think that if we use the textContrast configuration the values are readable and there is no need to also give the ability to the users to add the border.

Does it also make sense for Lens? :)

@stratoula it looks good to me
Probably, on our side, remain the ability to rotate the labels that can help on specific charts, but not much on readability.
Please also remember (and can be a nice suggestion on the UI somewhere) that rotating to horizontal bar is first, a good practice for categorical charts, and second can increase the space for the values.
The readability could benefit also from a less precise number formatter: for example, if the vertical axis shows K M G you can remove those multipliers from the bar values.

@stratoula stratoula added Feature:XYAxis XY-Axis charts (bar, area, line) v7.15.0 v8.0.0 release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Jul 20, 2021
@stratoula stratoula marked this pull request as ready for review July 20, 2021 09:28
@stratoula stratoula requested a review from a team July 20, 2021 09:28
@elasticmachine
Copy link
Contributor

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

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
visTypeXy 113.1KB 113.1KB +10.0B

History

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

@stratoula stratoula merged commit 17f00a8 into elastic:master Jul 21, 2021
stratoula added a commit to stratoula/kibana that referenced this pull request Jul 21, 2021
@stratoula
[XY axis] Fixes the values inside bar charts (elastic#106198)
f6f27e6 
* [XY axis] Fixes the values inside bar charts

* Apply PR comments
This was referenced Jul 21, 2021
Merged
Closed
stratoula added a commit that referenced this pull request Jul 21, 2021
@stratoula
[XY axis] Fixes the values inside bar charts (#106198) (#106370)
5270b83 
* [XY axis] Fixes the values inside bar charts

* Apply PR comments
jloleysens added a commit to jloleysens/kibana that referenced this pull request Jul 21, 2021
@jloleysens
Merge branch 'master' of github.com:elastic/kibana into reporting/onl…
5160a33 
…y-show-migrate-to-authzd-users

* 'master' of github.com:elastic/kibana: (48 commits)
  [Canvas] Expression shape (elastic#103219)
  [FTR] Skips Vega tests
  [Sample data] Use Lens in ecommerce data (elastic#106039)
  [APM] Backends inventory & overview page routes (elastic#106223)
  [TSVB] Add more functional tests for Gauge and TopN (elastic#105361)
  Add toggle to enable/disable rule install from SOs (elastic#106189)
  Improve unit test coverage of FS API calls (elastic#106242)
  Remove recursive plugin status in meta field (elastic#106286)
  [Ingest pipelines] add community id processor (elastic#103863)
  [XY axis] Fixes the values inside bar charts (elastic#106198)
  [data.search] Set default expiration to 1m if search sessions are disabled (elastic#105329)
  set the doc title when navigating to reporting and unset when navigating away (elastic#106253)
  [Lens] Display legend inside chart (elastic#105571)
  [RAC] [TGrid] Migrate the TGrid's rendering to `EuiDataGrid` (elastic#106199)
  [Security Solutions] Removes the elastic legacy client from lists and security_solution plugins (elastic#106130)
  [Enterprise Search] Require security plugin in 8.0 (elastic#106307)
  [DOCS] Updates screenshots in Dev Tools docs (elastic#105859)
  [DOCS] Updates text and screenshots in tags doc (elastic#105853)
  [Alerting] Allow rule types to extract/inject saved object references on rule CRU (elastic#101896)
  Jest and Storybook fixes (elastic#104991)
  ...

# Conflicts:
#	x-pack/plugins/reporting/public/plugin.ts
TokenPayId
TokenPayId reviewed Oct 11, 2023
View reviewed changes
Copy link

@TokenPayId TokenPayId left a comment

Choose a reason for hiding this comment

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

``#106198 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TokenPayId TokenPayId TokenPayId left review comments

@dej611 dej611 dej611 approved these changes

Assignees
No one assigned
Labels
Feature:XYAxis XY-Axis charts (bar, area, line) release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.15.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@stratoula @markov00 @elasticmachine @kibanamachine @dej611 @TokenPayId