Skip to content

[TSVB] Multi-metric gauge doesn't like vertical sizing#33245

Merged
alexwizp merged 6 commits intoelastic:masterfrom
alexwizp:tsvb_27770
Mar 15, 2019
Merged

[TSVB] Multi-metric gauge doesn't like vertical sizing#33245
alexwizp merged 6 commits intoelastic:masterfrom
alexwizp:tsvb_27770

Conversation

@alexwizp
Copy link
Copy Markdown
Contributor

@alexwizp alexwizp commented Mar 14, 2019 •
edited
Loading

Summary

This PR fix the problem which was described here #27770 for TSVB plugin

Before:

TSVB Gauge panel - Edit mode:

resize_before

TSVB Gauge panel - Dashboard:

resize_before_d

###After:

TSVB Gauge panel - Edit mode:

resize_after

TSVB Gauge panel - Dashboard:

resize_after_d

Checklist

For maintainers

@alexwizp alexwizp requested a review from a team as a code owner March 14, 2019 14:52
@alexwizp alexwizp mentioned this pull request Mar 14, 2019
Closed
cchaos
cchaos reviewed Mar 14, 2019
View reviewed changes
Copy link
Copy Markdown
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.

Can you explain more about what you are trying to fix?

@alexwizp
Copy link
Copy Markdown
Contributor Author

alexwizp commented Mar 14, 2019

@cchaos screens will be attached soon. Sorry =)

@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Mar 14, 2019

💚 Build Succeeded

cchaos
cchaos suggested changes Mar 14, 2019
View reviewed changes
Copy link
Copy Markdown
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.

This isn't quite working. It will cut off the top row if there's not enough height.

Screen Shot 2019-03-14 at 12 19 03 PM

And when you start shrinking the width it can even hide the full first row.

src/legacy/core_plugins/metrics/public/components/_vis_with_splits.scss Outdated
overflow: hidden;
&__split {
display: flex;
flex: 0 1 auto;
Copy link
Copy Markdown
Contributor

@cchaos cchaos Mar 14, 2019

Choose a reason for hiding this comment

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

Are you sure you want to restrict allowing the metric to grow? It means it will never get bigger than 200x200, which is the opposite of what a non-split chart will do:

Screen Shot 2019-03-14 at 12 33 43 PM

vs

Screen Shot 2019-03-14 at 12 33 03 PM

Copy link
Copy Markdown
Contributor Author

@alexwizp alexwizp Mar 14, 2019

Choose a reason for hiding this comment

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

@cchaos you are right, my mistake. I think I can revert 'flex: 1 0 auto;' but what do you think about next case?:
image

The last line bigger than previous one

@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Mar 14, 2019

💚 Build Succeeded

@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Mar 14, 2019

💚 Build Succeeded

@cchaos
Copy link
Copy Markdown
Contributor

cchaos commented Mar 14, 2019

Hmmm, yeah so I had to fiddle with this quite a bit. Flex box can be annoying with dynamic content. The best I could get I've sent you a PR for. Essentially It will start wrapping after 4 items, and if there's less than 4, they will grow.

It does the similar thing to what you're asking about which is that the last line could be bigger than the rest, but the sizing here seems to be a nice sweetspot where the panel would have to be quite tall before that last row would grow.

@alexwizp
Copy link
Copy Markdown
Contributor Author

alexwizp commented Mar 14, 2019 •
edited
Loading

@cchaos looks awesome! Thanks

@alexwizp alexwizp requested a review from cchaos March 14, 2019 18:05
cchaos
cchaos approved these changes Mar 14, 2019
View reviewed changes
Copy link
Copy Markdown
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.

Just checked in IE and FF and it works great there too!

Now, while this is a great improvement, I think the ideal situation is to actually ask the user how many items they want per line and that way we can properly calculate the size.

@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Mar 14, 2019

💚 Build Succeeded

@alexwizp alexwizp merged commit 795fd1b into elastic:master Mar 15, 2019
@alexwizp alexwizp mentioned this pull request Mar 15, 2019
Merged
alexwizp added a commit to alexwizp/kibana that referenced this pull request Mar 15, 2019
@alexwizp
[TSVB] Multi-metric gauge doesn't like vertical sizing (elastic#33245)
954dfeb
@alexwizp alexwizp mentioned this pull request Mar 15, 2019
Merged
alexwizp added a commit to alexwizp/kibana that referenced this pull request Mar 15, 2019
@alexwizp
[TSVB] Multi-metric gauge doesn't like vertical sizing (elastic#33245)
83a3904
alexwizp added a commit that referenced this pull request Mar 15, 2019
@alexwizp
[TSVB] Multi-metric gauge doesn't like vertical sizing (#33245) (#33286)
6e78779
alexwizp added a commit that referenced this pull request Mar 15, 2019
@alexwizp
[TSVB] Multi-metric gauge doesn't like vertical sizing (#33245) (#33285)
4d5b010
@alexwizp alexwizp deleted the tsvb_27770 branch January 4, 2020 08:10
@VladLasitsa VladLasitsa mentioned this pull request Aug 20, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@markov00 markov00 Awaiting requested review from markov00

@timroes timroes Awaiting requested review from timroes

@flash1293 flash1293 Awaiting requested review from flash1293

1 more reviewer

@cchaos cchaos cchaos approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Feature:TSVB TSVB (Time Series Visual Builder) release_note:fix v7.0.0 v7.2.0 v8.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@alexwizp @elasticmachine @cchaos