-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
[Controls] [Reporting] Remove controls from reports #134240
[Controls] [Reporting] Remove controls from reports #134240
Conversation
Pinging @elastic/kibana-presentation (Team:Presentation) |
Pinging @elastic/kibana-reporting-services (Team:Reporting Services) |
💚 Build SucceededMetrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: cc @Heenawter |
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
Thanks for the details notes - I agree with everything you stated!
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.
As we discussed, this fix looks good! I just wonder if the title of the PR reflects what it's actually doing, left a comment to that effect. LGTM!
@@ -115,7 +115,6 @@ export const ControlGroup = () => { | |||
responsive={false} | |||
alignItems="center" | |||
data-test-subj="controls-group" | |||
data-shared-items-count={idsInOrder.length} |
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.
Does this change actually "Remove controls from reports"? Or does it simply fix the loading issue we were encountering before?
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.
I mean, it does remove controls from reports (specifically, non-print-optimized reports) - they used to be screen captured and included in these reports, and now they are not and the reports only include the graphs on the dashboard. I could change the title to reflect that this only impacts non-print-optimized reports?
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.
Oh! I didn't realise that simply removing the data-shared-items-count
would actually remove the Controls from the report! In that case the title makes total sense. Thanks for the response!
Friendly reminder: Looks like this PR hasn’t been backported yet. |
Closes #133547
Summary
Before this change, controls were not included in print optimized reports, but they were included in non-print-optimized reports. However, this caused a bug where sometimes the reporting states didn't match - more context can be found here.
To fix this, I removed the
data-shared-items-count
from thecontrol_group_component
, because it was found that this was causing the ambiguous state for Reporting since it only expects a singledata-shared-items-count
for the entire page. This solution removes controls from non-print-optimized reports entirely.Note that, if we wanted to include controls in reports in the future, we would have to pull the
dashboard-shared-items-container
class outside to include both thecontrol_group
and theDashboardGrid
, with something like:However, since we don't have a strong preference for whether or not controls should be visible in non-print-optimized reports anyway, the simplest and most reliable solution is to simply to not include them at all for now.
How to test:
Checklist