-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Fix #21355, visualization controls showing up in reports #21362
Fix #21355, visualization controls showing up in reports #21362
Conversation
💚 Build Succeeded |
.vis-editor-content | ||
.default-vis-editor | ||
> :not(.vis-editor-canvas):not(.vis_editor) { | ||
visualize-app visualize > :not(.vis-editor-content):not(.vis_editor), /* all non-content rows in interface */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since there is no visualize
element anymore, that rule should not work. But to be honest, I had no idea what it tried to hide earlier (maybe TSVB editor?).
Do we know what change(s) caused this? |
💔 Build Failed |
@LeeDr The editor UI breakage, was caused by #20263. The breakage of some minor issues also fixed via this like legend toggle button, seemed to be broken for quiet some time. Whatever breakage caused the reporting tests to be completly disabled is something Stacey might answer best. The banner breakage was most likely always present since the banner. |
I suspect a flaky test: jenkins, test this |
Reporting snapshot tests were turned off before the xpack merge into OSS. They were breaking so frequently that it was blocking developers, and since they were broken due to changes in OSS, it was async and costing a lot of time for everyone. We had to turn them off and planned to turn them back on after the merge. We've been making headway with various expansion of the reporting tests in general (e.g. we finally have chromium tests running), but still don't have the snapshot tests turned back on. Will try to focus on it asap. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Did a code review, also pulled down the changes and tested with phantom and chromium, visualization reports and dashboard, also a vega and TSVB visualization.
Confirmed I could first repro the bug on master.
Regarding the banner breakage, it might be good to get a rule in place that essentially says, "Hide everything that's not in a ".reportable" element. That way, if we add any other global banners or things in the future, they won't ever show in reports. |
💚 Build Succeeded |
.default-vis-editor | ||
> :not(.vis-editor-canvas):not(.vis_editor) { | ||
display: none; | ||
visualize-app visualize > :not(.vis-editor-content):not(.vis_editor), /* all non-content rows in interface */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as below, there is no visualize
element anymore.
…tic#21362) Hide visualization controls and the feedback banner in reports
…tic#21362) Hide visualization controls and the feedback banner in reports
Fixes #21355, per some suggestions from Tim. See the linked issue for repro steps.
I also noticed that the "Help us improve Elastic..." banner was showing up in reports, so this also removes that.