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][Timelion][XY] Cursor synchronization is not visible #116754

Closed
stratoula opened this issue Oct 29, 2021 · 9 comments · Fixed by #117180
Closed

[TSVB][Timelion][XY] Cursor synchronization is not visible #116754

stratoula opened this issue Oct 29, 2021 · 9 comments · Fixed by #117180
Assignees
Labels
bug Fixes for quality problems that affect the customer experience Feature:TSVB TSVB (Time Series Visual Builder) impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. Team:Visualizations Visualization editors, elastic-charts and infrastructure

Comments

@stratoula
Copy link
Contributor

stratoula commented Oct 29, 2021

Kibana version:
7.16+

Describe the bug:
This bug is more obvious when you add a TSVB and a Lens chart on a dahboard. If you hover over TSVB, the hover line is not visible but you will see that the it is synchronized with the Lens chart. If you hover over Lens and check TSVB you will see no indication of the cursor synchronization.

I think that it works but the line is invisible for some reason. In 7.15 it looked like:
image

but in 7.16 (same dashboard)
image

Steps to reproduce:

  1. Create a new dashboard
  2. Add a Lens timeseries chart
  3. Add a TSVB timeseries chart or a timelion agg-based chart
  4. Hover over Lens, no indication of the synchronization on TSVB
@stratoula stratoula added bug Fixes for quality problems that affect the customer experience Feature:TSVB TSVB (Time Series Visual Builder) Team:Visualizations Visualization editors, elastic-charts and infrastructure impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. labels Oct 29, 2021
@elasticmachine
Copy link
Contributor

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

@alexwizp
Copy link
Contributor

@nickofthyme could you please help us a little to understand what has changed in the charts, I see again problems with theme styles merging.

e.g. if I remove "chartTheme" from the TSVB the cursor became to be visible.
image

image

@nickofthyme
Copy link
Contributor

Yeah I'll take a look.

@stratoula stratoula changed the title [TSVB] Cursor synchronization is not visible [TSVB][Timelion] Cursor synchronization is not visible Nov 1, 2021
@stratoula
Copy link
Contributor Author

stratoula commented Nov 1, 2021

This also happens in Timelion and XY charts. Is this on EC side or can we fix it on our side? I think it should be fixed in 7.16 cc @nickofthyme

@stratoula stratoula changed the title [TSVB][Timelion] Cursor synchronization is not visible [TSVB][Timelion][XY] Cursor synchronization is not visible Nov 1, 2021
@nickofthyme
Copy link
Contributor

Ugh I thought this was the same issue related to undefined base properties. Basically if base theme does not define a default value for properties that are possibly undefined, it doesn't pickup overrides to that value.

But this issue appears to be related to the value being an optional array, namely theme.crosshair.line.dash. I'm looking into a fix in kibana now.

@nickofthyme
Copy link
Contributor

Alright after a deeper look this issue here is caused when the first partial theme in the array is empty ({}). But since this is recursive this means empty at any level within the theme object tree, which is easy to run into.

In such a case the additional theme objects in the array are completely ignored.

I don't see a good solution for this on kibana side. I will put up a PR and backport it into 7.16 as a patch fix if that would work for you.

@stratoula
Copy link
Contributor Author

Thank you @nickofthyme, you rock 🚀

@nickofthyme
Copy link
Contributor

nickofthyme commented Nov 2, 2021

Ok @stratoula & @alexwizp, #117180 should fix this issue in its entirety. Please let me know if you see any issues.

So now that the theming is fixed I hope there is less confusion in how the theme merging works. Essentially it takes looks to set each value within the theme from the partial or array of partials set on the Settings.theme prop, looking to each successive partial for every value. If none of the partials contain the given property the value will be pulled from the full baseTheme. I am happy to jump on a call to explain in more detail if needed.

@nickofthyme
Copy link
Contributor

This fix is now in main as of @elastic/[email protected] or #117619.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Feature:TSVB TSVB (Time Series Visual Builder) impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. Team:Visualizations Visualization editors, elastic-charts and infrastructure
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants