[Observability] fix slo observability compressed styles to be not compressed #193081

rshen91 · 2024-09-16T19:54:49Z

Summary

Building off of PR #192993 to revert the compressed styles for SLOs

Compressed styles PR here

Before

In the SLO page in Observability, the status and tags fields are uneven with the unified search bar.

After

...ck/plugins/observability_solution/slo/public/pages/slos/components/common/quick_filters.scss

elasticmachine · 2024-09-17T18:50:15Z

Pinging @elastic/obs-ux-management-team (Team:obs-ux-management)

src/plugins/controls/public/react_controls/control_group/components/control_renderer.tsx

Heenawter · 2024-09-17T19:39:00Z

...ck/plugins/observability_solution/slo/public/pages/slos/components/common/quick_filters.scss

@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.

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

i still honestly don't understand why these custom styles are kept in SLO plugin, they should be moved where the control components are.

@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

@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 🙇

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

…-compressed

MichaelMarcialis · 2024-09-30T16:23:16Z

@rshen91: Any chance you could include before/after sceenshots for this change to help reviewers? Thanks!

nreese · 2024-09-30T19:06:08Z

src/plugins/controls/public/control_group/control_group_renderer/control_group_renderer.tsx

      })}
      onApiAvailable={(controlGroupApi) => {
        const controlGroupRendererApi: ControlGroupRendererApi = {
          ...controlGroupApi,
+          compressed: compressed ?? true,


This line is not needed. compressed should not be something exposed from the ControlGroupApi. Its only used internally by the control group. Its value comes from the parent. Consumers do not need to know about this implementation detail.

Heenawter · 2024-10-01T17:48:08Z

packages/presentation/presentation_publishing/interfaces/has_compressed.ts

FYI I think these types / type guards should live under the control_group folder as well - I don't think we should add something so specific to controls under presentation_publishing :)

nreese · 2024-10-01T17:51:40Z

packages/presentation/presentation_publishing/interfaces/has_compressed.ts

+export const apiHasCompressed = (unknownApi: unknown): unknownApi is HasCompressed => {
+  return (
+    (unknownApi as HasCompressed).compressed !== undefined &&
+    typeof (unknownApi as HasCompressed).compressed === 'function'


Type should verify 'boolean', not 'function'. Also, lets put this in controls plugin, since only controls uses this interface for now

nreese · 2024-10-01T17:53:00Z

src/plugins/controls/public/control_group/utils/is_compressed.ts

+export const isCompressed = (
+  api: DefaultControlApi | HasParentApi | HasCompressed | null | unknown
+): boolean => {
+  if (apiHasCompressed(api)) {


This is not quite right. Try this instead

function isCompressed(api: unknown) { if (apiHasCompressed(api)) return api.compressed; return apiHasParentApi(api) ? isCompressed(api.parent) : true; }

nreese · 2024-10-01T18:32:46Z

src/plugins/controls/public/control_group/utils/has_compressed.ts

@@ -0,0 +1,19 @@
+/*


How about moving this into is_compressed.ts?

nreese · 2024-10-01T18:33:50Z

src/plugins/controls/public/control_group/utils/has_compressed.ts

+  compressed: boolean;
+}
+
+export const apiHasCompressed = (unknownApi: unknown): unknownApi is HasCompressed => {


type guards should check for undefined. So you guard should look like Boolean(unknownApi && typeof (unknownApi as HasCompressed).compressed === 'boolean'

nreese · 2024-10-01T18:34:07Z

src/plugins/controls/public/control_group/utils/is_compressed.ts

+import { apiHasCompressed } from './has_compressed';
+
+export function isCompressed(api: unknown): boolean {
+  if (api === null) return true;


No need to check for null. The type guards check that api is defined.

Heenawter · 2024-10-01T19:16:55Z

src/plugins/controls/public/control_group/utils/is_compressed.ts

+export interface HasCompressed {
+  compressed: boolean;
+}
+
+export const apiHasCompressed = (unknownApi: unknown): unknownApi is HasCompressed => {
+  return Boolean(unknownApi) && typeof (unknownApi as HasCompressed).compressed === 'boolean';
+};


I don't think we need to export these

Heenawter · 2024-10-01T19:17:43Z

...trols/public/controls/data_controls/options_list_control/components/options_list_control.tsx

I believe the range slider + time slider controls also use the compressed prop on their components, no? We should make this consistent, even if the SLO page does not use these control types

Sounds good - I'm down for consistency bb77b0d for changes

src/plugins/controls/public/control_group/utils/is_compressed.ts

Heenawter

Thanks for adding the unit tests 🎉 They are very clean. LGTM - code review + tested and ensured only the SLO page is rendering with compressed = false

kibana-ci · 2024-10-01T22:18:58Z

💚 Build Succeeded

Buildkite Build
Commit: 00c0670
Kibana Serverless Image: docker.elastic.co/kibana-ci/kibana-serverless:pr-193081-00c06707dd7a

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`controls`	355	356	+1

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`controls`	130	131	+1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`controls`	458.9KB	459.5KB	+624.0B
`slo`	855.3KB	855.3KB	+14.0B
total			+638.0B

Unknown metric groups

API count

id	before	after	diff
`controls`	134	135	+1

History

💚 Build #238557 succeeded bb77b0d
💚 Build #238522 succeeded 899b248
💔 Build #238427 failed 83da3f5
💔 Build #238217 failed cf410d8
💔 Build #238200 failed 4cf021a

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

cc @rshen91

kibanamachine · 2024-10-01T23:01:43Z

Starting backport for target branches: 7.17, 8.15, 8.x

https://github.com/elastic/kibana/actions/runs/11134794003

…pressed (elastic#193081) ## Summary Building off of PR elastic#192993 to revert the compressed styles for SLOs Compressed styles PR [here](elastic#190636) ### Before In the SLO page in Observability, the status and tags fields are uneven with the unified search bar. ![Screenshot 2024-09-30 at 2 10 17 PM](https://github.com/user-attachments/assets/63391aa2-ec7d-43f5-9803-8094e73b8a6c) ### After ![Screenshot 2024-09-30 at 2 52 37 PM](https://github.com/user-attachments/assets/fb70109d-15d1-4278-81c6-318da290594f) (cherry picked from commit 7a9a519)

kibanamachine · 2024-10-01T23:06:41Z

💔 Some backports could not be created

Status	Branch	Result
❌	7.17	Backport failed because of merge conflicts You might need to backport the following PRs to 7.17: - Update ftr (main) (#194509)
❌	8.15	Backport failed because of merge conflicts
✅	8.x

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

Manual backport

To create the backport manually run:

node scripts/backport --pr 193081

Questions ?

Please refer to the Backport tool documentation

…ot compressed (#193081) (#194650) # Backport This will backport the following commits from `main` to `8.x`: - [[Observability] fix slo observability compressed styles to be not compressed (#193081)](#193081)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Rachel Shen <[email protected]>

fix slo observability compressed styles to be not compressed

2fb8a5a

rshen91 self-assigned this Sep 16, 2024

rshen91 requested a review from shahzad31 September 16, 2024 19:55

rshen91 added 4 commits September 16, 2024 14:03

remove sass changes from controls plugin

67ea6a2

update

cf01d44

fix

c9d3ffe

Merge remote-tracking branch 'upstream/main' into slo-non-compressed

0b6fbc0

shahzad31 approved these changes Sep 17, 2024

View reviewed changes

rshen91 marked this pull request as ready for review September 17, 2024 18:48

rshen91 requested review from a team as code owners September 17, 2024 18:48

rshen91 added the release_note:skip Skip the PR/issue when compiling release notes label Sep 17, 2024

shahzad31 reviewed Sep 17, 2024

View reviewed changes

...ck/plugins/observability_solution/slo/public/pages/slos/components/common/quick_filters.scss Outdated Show resolved Hide resolved

botelastic bot added ci:project-deploy-observability Create an Observability project Team:obs-ux-management Observability Management User Experience Team labels Sep 17, 2024

Heenawter reviewed Sep 17, 2024

View reviewed changes

src/plugins/controls/public/react_controls/control_group/components/control_renderer.tsx Outdated Show resolved Hide resolved

Heenawter reviewed Sep 17, 2024

View reviewed changes

...ck/plugins/observability_solution/slo/public/pages/slos/components/common/quick_filters.scss Outdated Show resolved Hide resolved

rshen91 added 5 commits September 18, 2024 15:36

code review

8c45a2a

Merge remote-tracking branch 'upstream/main' into slo-non-compressed

5354eb0

Merge remote-tracking branch 'upstream/main' into slo-non-compressed

e590230

code review sass clean up

caa0bf2

Merge remote-tracking branch 'origin/slo-non-compressed' into slo-non…

7f0d2f3

…-compressed

rshen91 added the backport:all-open Backport to all branches that could still receive a release label Sep 20, 2024

Merge remote-tracking branch 'upstream/main' into slo-non-compressed

2831847

wip refactor

a722e87

nreese reviewed Sep 30, 2024

View reviewed changes

rshen91 added 6 commits September 30, 2024 16:27

fix

cf410d8

Merge remote-tracking branch 'upstream/main' into slo-non-compressed

9c60c87

fix

83da3f5

cleaner

02f6a67

Merge remote-tracking branch 'upstream/main' into slo-non-compressed

9e32829

fix typeguard type

c986f23

Heenawter reviewed Oct 1, 2024

View reviewed changes

fix

ad6ee2c

nreese reviewed Oct 1, 2024

View reviewed changes

code review

899b248

nreese reviewed Oct 1, 2024

View reviewed changes

code review

adc1846

rshen91 requested review from nreese and Heenawter October 1, 2024 19:01

Heenawter reviewed Oct 1, 2024

View reviewed changes

code review

bb77b0d

Heenawter reviewed Oct 1, 2024

View reviewed changes

src/plugins/controls/public/control_group/utils/is_compressed.ts Show resolved Hide resolved

rshen91 added 2 commits October 1, 2024 15:18

unit tests

b21f593

Merge remote-tracking branch 'upstream/main' into slo-non-compressed

00c0670

Heenawter approved these changes Oct 1, 2024

View reviewed changes

rshen91 enabled auto-merge (squash) October 1, 2024 21:30

rshen91 merged commit 7a9a519 into elastic:main Oct 1, 2024
23 checks passed

kibanamachine added the v9.0.0 label Oct 1, 2024

kibanamachine mentioned this pull request Oct 1, 2024

[8.x] [Observability] fix slo observability compressed styles to be not compressed (#193081) #194650

Merged

kibanamachine added the v8.16.0 label Oct 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Observability] fix slo observability compressed styles to be not compressed #193081

[Observability] fix slo observability compressed styles to be not compressed #193081

rshen91 commented Sep 16, 2024 •

edited by kibanamachine

Loading

elasticmachine commented Sep 17, 2024

Heenawter Sep 17, 2024 •

edited

Loading

rshen91 Sep 20, 2024

shahzad31 Sep 23, 2024

nreese Sep 23, 2024 •

edited

Loading

Heenawter Sep 23, 2024 •

edited

Loading

nreese Sep 23, 2024

MichaelMarcialis commented Sep 30, 2024

nreese Sep 30, 2024

Heenawter Oct 1, 2024

nreese Oct 1, 2024

nreese Oct 1, 2024 •

edited

Loading

nreese Oct 1, 2024

nreese Oct 1, 2024

nreese Oct 1, 2024

Heenawter Oct 1, 2024

Heenawter Oct 1, 2024

rshen91 Oct 1, 2024 •

edited

Loading

Heenawter left a comment

kibana-ci commented Oct 1, 2024 •

edited

Loading

API count

kibanamachine commented Oct 1, 2024

kibanamachine commented Oct 1, 2024

[Observability] fix slo observability compressed styles to be not compressed #193081

[Observability] fix slo observability compressed styles to be not compressed #193081

Conversation

rshen91 commented Sep 16, 2024 • edited by kibanamachine Loading

Summary

Before

After

elasticmachine commented Sep 17, 2024

Heenawter Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nreese Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

Heenawter Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelMarcialis commented Sep 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nreese Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rshen91 Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

Heenawter left a comment

Choose a reason for hiding this comment

kibana-ci commented Oct 1, 2024 • edited Loading

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

API count

History

kibanamachine commented Oct 1, 2024

kibanamachine commented Oct 1, 2024

💔 Some backports could not be created

Manual backport

Questions ?

rshen91 commented Sep 16, 2024 •

edited by kibanamachine

Loading

Heenawter Sep 17, 2024 •

edited

Loading

nreese Sep 23, 2024 •

edited

Loading

Heenawter Sep 23, 2024 •

edited

Loading

nreese Oct 1, 2024 •

edited

Loading

rshen91 Oct 1, 2024 •

edited

Loading

kibana-ci commented Oct 1, 2024 •

edited

Loading