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.

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

[TSVB] Hides ticks on the y axis for layers with the same format and different template #123598

Merged
merged 4 commits into from
Jan 27, 2022

Conversation

alexwizp
Copy link
Contributor

@alexwizp alexwizp commented Jan 24, 2022

Closes: #123439

Describe the bug:

The bug exists if you have a TSVB timeseries chart with more than 1 layers.

Expected behavior:

  • If the formatters are the same (here for example both layers are using percent) and the value template changes we should depict it on the yaxis. We can follow Lens example and display the data with the first template

  • If the formatters are different (for example one percent, one default), the y axis should depict the first formatting (as Lens does)

  • In TSVB, this was originally implemented differently from LENS. In case the formatters are different, we use the default number formatter

Copy link
Contributor

@DianaDerevyankina DianaDerevyankina left a comment

Choose a reason for hiding this comment

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

LGTM! Works as expected, tested locally

@alexwizp
Copy link
Contributor Author

@elasticmachine merge upstream

@stratoula stratoula added Feature:TSVB TSVB (Time Series Visual Builder) Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 v8.1.0 release_note:fix labels Jan 26, 2022
@stratoula stratoula marked this pull request as ready for review January 26, 2022 07:58
@stratoula stratoula requested a review from a team as a code owner January 26, 2022 07:58
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-vis-editors @elastic/kibana-vis-editors-external (Team:VisEditors)

@stratoula
Copy link
Contributor

@elasticmachine merge upstream

@stratoula
Copy link
Contributor

I am testing it by comparing TSVB v7.11 with this branch. One thing that works differently:

  • I have 2 layers on the same axis with Bytes formatting. Everything works fine. When I change the template of the first layer to {{value/s}} then the formatting disappears and I see the default formatting.
    image

On 7.11 the formatting persists and the template of the first series is applied
image

@stratoula
Copy link
Contributor

@alexwizp this seems to work as before now.

@flash1293 do you want to also take a look?
When we had discussed it, I thought that TSVB used to work like Lens, if there are 2 layers with different formatting on the same axis, the formatting of the first layer is applied to the axis.

But I checked it on v7.11 and I saw that if you have one layer with percent for example and the second with bytes, no formatter is applied on the yaxis.

I prefer the Lens way but I think that we should be bwc and not change the behavior. Wdyt?

@elastic elastic deleted a comment from kibana-ci Jan 26, 2022
@flash1293
Copy link
Contributor

flash1293 commented Jan 26, 2022

Good call about not breaking BWC here - it's kind of an edge case though, no strong opinion. I'm happy with the approach in general, there is one thing I noticed though:

This seems to be a separate bug, but if two different formatters (not just different template) are used on two separate axes, the first axis still uses the default formatter:
(count is on the left axis, tooltip is formatted right, but left axis isn't)
Screenshot 2022-01-26 at 16 44 32

it works if the same formatter is used on both (with separate template):
Screenshot 2022-01-26 at 16 45 14

Should we split it out or fix this on this PR?

@stratoula
Copy link
Contributor

stratoula commented Jan 26, 2022

@flash1293 yes I know about this too. If you set on the first layer to use a separate left axis, then the formatter is applied....
This is how it worked on 7.11

Screenshot from 7.11 (right axis works fine, left axis doesn't have the format)
image

With that being said, I think we could create an issue for that and tackle it to a different PR

@flash1293
Copy link
Contributor

Got it, looks good to me in that case.

@kibana-ci
Copy link
Collaborator

💚 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
visTypeTimeseries 447.8KB 447.9KB +74.0B

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

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, this works as expected. I created another issue for the behavior that Joe mentions here
#123891

@alexwizp alexwizp added the auto-backport Deprecated - use backport:version if exact versions are needed label Jan 27, 2022
@alexwizp alexwizp merged commit 61ef26d into elastic:main Jan 27, 2022
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jan 27, 2022
…different template (elastic#123598)

* [TSVB] Hides ticks on the y axis for layers with the same format and different template

* fix PR comment

* fix JEST

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
(cherry picked from commit 61ef26d)
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jan 27, 2022
…different template (elastic#123598)

* [TSVB] Hides ticks on the y axis for layers with the same format and different template

* fix PR comment

* fix JEST

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
(cherry picked from commit 61ef26d)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.0
7.17

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

@alexwizp alexwizp added backport:skip This commit does not require backporting and removed v8.0.0 v7.17.0 auto-backport Deprecated - use backport:version if exact versions are needed labels Jan 27, 2022
stratoula pushed a commit to stratoula/kibana that referenced this pull request Feb 14, 2022
…different template (elastic#123598)

* [TSVB] Hides ticks on the y axis for layers with the same format and different template

* fix PR comment

* fix JEST

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
(cherry picked from commit 61ef26d)
stratoula pushed a commit to stratoula/kibana that referenced this pull request Feb 14, 2022
…different template (elastic#123598)

* [TSVB] Hides ticks on the y axis for layers with the same format and different template

* fix PR comment

* fix JEST

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
(cherry picked from commit 61ef26d)
stratoula added a commit that referenced this pull request Feb 16, 2022
…different template (#123598) (#125515)

* [TSVB] Hides ticks on the y axis for layers with the same format and different template

* fix PR comment

* fix JEST

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
(cherry picked from commit 61ef26d)

Co-authored-by: Alexey Antonov <alexwizp@gmail.com>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
stratoula added a commit that referenced this pull request Feb 16, 2022
…different template (#123598) (#125516)

* [TSVB] Hides ticks on the y axis for layers with the same format and different template

* fix PR comment

* fix JEST

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
(cherry picked from commit 61ef26d)

Co-authored-by: Alexey Antonov <alexwizp@gmail.com>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:TSVB TSVB (Time Series Visual Builder) release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TSVB] Hides ticks on the y axis for layers with the same format and different template
7 participants