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

Improve bar value styles on vis_type_xy charts #87991

Merged
merged 2 commits into from
Jan 15, 2021
Merged

Improve bar value styles on vis_type_xy charts #87991

merged 2 commits into from
Jan 15, 2021

Conversation

VladLasitsa
Copy link
Contributor

@VladLasitsa VladLasitsa commented Jan 12, 2021 •
edited
Loading

Summary

Closes #82853
Added the same defaults for bar value styles from Lens to vis_type_xy charts.

How it looks with these defaults you can see here:
image

For maintainers

@VladLasitsa VladLasitsa self-assigned this Jan 12, 2021
nickofthyme
nickofthyme reviewed Jan 12, 2021
View reviewed changes
src/plugins/vis_type_xy/public/components/xy_settings.tsx
Comment on lines +83 to +91
fontSize: { min: VALUE_LABELS_MIN_FONTSIZE, max: VALUE_LABELS_MAX_FONTSIZE },
fill: { textInverted: true, textBorder: 2 },
alignment: isHorizontal
? {
vertical: VerticalAlignment.Middle,
}
: { horizontal: HorizontalAlignment.Center },
offsetX: isHorizontal ? VALUE_LABELS_HORIZONTAL_OFFSET : 0,
offsetY: isHorizontal ? 0 : VALUE_LABELS_VERTICAL_OFFSET,
Copy link
Contributor

Choose a reason for hiding this comment

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

These values should all come from the theme object. This will override all of these properties and ignore the value defined in the theme.

You should be able to add these overrides via the Settings.theme prop already. What is the use case for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I understood we should use defaults from Lens for values styles. These values which I found in Lens we should use in plugin. As we already have 'themeOverrides' variable I think we should add these defaults there. For convenience I created a separate function for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wow, sorry 😐. When I looked at this PR I thought this was done in @elastic/charts. Just ignore me and my comment this is perfect!

alexwizp
alexwizp approved these changes Jan 13, 2021
View reviewed changes
Copy link
Contributor

@alexwizp alexwizp left a comment

Choose a reason for hiding this comment

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

LGTM in case of updating description

@VladLasitsa VladLasitsa added v7.12.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Jan 14, 2021
@VladLasitsa VladLasitsa marked this pull request as ready for review January 14, 2021 11:48
@VladLasitsa VladLasitsa requested a review from a team January 14, 2021 11:48
@elasticmachine
Copy link
Contributor

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

@VladLasitsa
Copy link
Contributor Author

@elasticmachine merge upstream

@stratoula
Copy link
Contributor

@VladLasitsa can you also add a screenshot on the description, please? 🙏

@stratoula
Copy link
Contributor

stratoula commented Jan 14, 2021 •
edited
Loading

@ryankeairns what do you think? We use the same configuration as Lens but just to be in sync here 🙂

image

My only concern is that it is not so responsive, especially if you have more than one y axis.
image

@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 157.7KB 157.4KB -338.0B

History

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

@ryankeairns
Copy link
Contributor

@stratoula apologies for the delay in replying. It looks like Lens ran into similar concerns and opted to wait until there is better support from Elastic Charts. #81776 (comment)

Should the same approach be taken here? Things do seem to get unwieldy fast.

@nickofthyme
Copy link
Contributor

I agree we need to handle the edge cases better in elastic charts. I think this is a good start for now.

stratoula
stratoula approved these changes Jan 15, 2021
View reviewed changes
Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

Code LGTM. It works well in most cases. Thanx @ryankeairns and @nickofthyme for your feedback. I agree Nick, it is a good start 🙂

@VladLasitsa VladLasitsa merged commit 5fd0027 into elastic:master Jan 15, 2021
@VladLasitsa VladLasitsa mentioned this pull request Jan 15, 2021
Merged
VladLasitsa added a commit to VladLasitsa/kibana that referenced this pull request Jan 15, 2021
@VladLasitsa @kibanamachine
use the same defaults as Lens (elastic#87991)
f2ffe41 
Co-authored-by: Kibana Machine <[email protected]>
VladLasitsa added a commit that referenced this pull request Jan 18, 2021
@VladLasitsa @kibanamachine
use the same defaults as Lens (#87991) (#88461)
80a7613 
Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Kibana Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stratoula stratoula stratoula approved these changes

@nickofthyme nickofthyme nickofthyme approved these changes

@alexwizp alexwizp alexwizp approved these changes

Assignees

@VladLasitsa VladLasitsa

Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.12.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve bar value styles on vis_type_xy charts
7 participants
@VladLasitsa @elasticmachine @stratoula @kibanamachine @ryankeairns @nickofthyme @alexwizp