-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Observability] fix slo observability compressed styles to be not compressed #193081
base: main
Are you sure you want to change the base?
Conversation
...ck/plugins/observability_solution/slo/public/pages/slos/components/common/quick_filters.scss
Outdated
Show resolved
Hide resolved
Pinging @elastic/obs-ux-management-team (Team:obs-ux-management) |
src/plugins/controls/public/react_controls/control_group/components/control_renderer.tsx
Outdated
Show resolved
Hide resolved
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.
@shahzad31 Can you confirm that it is just the SLO page that should have non-compressed styling?
From my understanding, only the SLO page's controls should be impacted - but right now, the Alerts
page also uses this QuickFilters
component, so the styles have been expanded here, too (and I believe they explicitly prefer the compact styling).
@rshen If possible, I think it would be better to only target the SLO page and not the whole QuickFilters
component since this is used a lot more places than just SLO.
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.
Sorry - I think this is the SLO page, totally can be misunderstanding though 😶🌫️ ? x-pack/plugins/observability_solution/slo/public/pages/slos/components/common/quick_filters.scss
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 still honestly don't understand why these custom styles are kept in SLO plugin, they should be moved where the control components are.
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.
@rshen91 One solution to remove custom styles from SLO would be to add a compressed
prop to ControlGroupRenderer
. Then, ControlGroupRenderer
could pass compressed
to control group embeddable by adding a compressed
boolean in the parent returned from ReactEmbeddableRenderer.getParentApi
. Finally, the control group embeddable could use compressed styles if the parentApi contains compressed
key where typeof parentApi.compressed === boolean && parentApi.compressed
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.
@rshen91 @shahzad31 fyi What @nreese is describing is a potential solution for #189939 that he came up with and we discussed offline :) I think it could work really well for a case like this - we can add a compressed
prop to ControlGroupRenderer
without having to add "serializable state" to the control group, which is what we were originally trying to avoid with these custom styles.
I think this is the best plan moving forward - sorry for the back and forth 🙇
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.
Here is an example for MapRenderer that migrates props from serialized state to parent Api.
...ck/plugins/observability_solution/slo/public/pages/slos/components/common/quick_filters.scss
Outdated
Show resolved
Hide resolved
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: cc @rshen91 |
Summary
Building off of PR #192993 to revert the compressed styles for SLOs