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.

Sign up for GitHub

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

Merged
merged 29 commits into from
Oct 1, 2024
Merged

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
2fb8a5a
fix slo observability compressed styles to be not compressed
rshen91 Sep 16, 2024
67ea6a2
remove sass changes from controls plugin
rshen91 Sep 16, 2024
cf01d44
update
rshen91 Sep 16, 2024
c9d3ffe
fix
rshen91 Sep 17, 2024
0b6fbc0
Merge remote-tracking branch 'upstream/main' into slo-non-compressed
rshen91 Sep 17, 2024
8c45a2a
code review
rshen91 Sep 18, 2024
5354eb0
Merge remote-tracking branch 'upstream/main' into slo-non-compressed
rshen91 Sep 18, 2024
e590230
Merge remote-tracking branch 'upstream/main' into slo-non-compressed
rshen91 Sep 20, 2024
caa0bf2
code review sass clean up
rshen91 Sep 20, 2024
7f0d2f3
Merge remote-tracking branch 'origin/slo-non-compressed' into slo-non…
rshen91 Sep 20, 2024
2831847
Merge remote-tracking branch 'upstream/main' into slo-non-compressed
rshen91 Sep 30, 2024
a722e87
wip refactor
rshen91 Sep 30, 2024
913f0ff
revert some changes
rshen91 Sep 30, 2024
bb6a928
Merge remote-tracking branch 'upstream/main' into slo-non-compressed
rshen91 Sep 30, 2024
451d33c
remove from controls api
rshen91 Sep 30, 2024
c9a2d59
fix
rshen91 Sep 30, 2024
4cf021a
Merge remote-tracking branch 'upstream/main' into slo-non-compressed
rshen91 Sep 30, 2024
cf410d8
fix
rshen91 Sep 30, 2024
9c60c87
Merge remote-tracking branch 'upstream/main' into slo-non-compressed
rshen91 Oct 1, 2024
83da3f5
fix
rshen91 Oct 1, 2024
02f6a67
cleaner
rshen91 Oct 1, 2024
9e32829
Merge remote-tracking branch 'upstream/main' into slo-non-compressed
rshen91 Oct 1, 2024
c986f23
fix typeguard type
rshen91 Oct 1, 2024
ad6ee2c
fix
rshen91 Oct 1, 2024
899b248
code review
rshen91 Oct 1, 2024
adc1846
code review
rshen91 Oct 1, 2024
bb77b0d
code review
rshen91 Oct 1, 2024
b21f593
unit tests
rshen91 Oct 1, 2024
00c0670
Merge remote-tracking branch 'upstream/main' into slo-non-compressed
rshen91 Oct 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions src/plugins/controls/public/react_controls/control_group/components/control_panel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,12 +154,10 @@ export const ControlPanel = <ApiType extends DefaultControlApi = DefaultControlA
data-test-subj="control-frame-title"
fullWidth
label={usingTwoLineLayout ? panelTitle || defaultPanelTitle || '...' : undefined}
display="rowCompressed"
>
<EuiFormControlLayout
fullWidth
isLoading={Boolean(dataLoading)}
compressed
className={classNames(
'controlFrame__formControlLayout',
{
Expand Down Expand Up @@ -191,6 +189,7 @@ export const ControlPanel = <ApiType extends DefaultControlApi = DefaultControlA
)}
</>
}
compressed
>
<>
{blockingError && (
Expand Down
4 changes: 3 additions & 1 deletion src/plugins/controls/public/react_controls/control_group/components/control_renderer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,14 @@ export const ControlRenderer = <
getParentApi,
onApiAvailable,
isControlGroupInitialized,
className,
}: {
type: string;
uuid: string;
getParentApi: () => ControlGroupApi;
onApiAvailable?: (api: ApiType) => void;
isControlGroupInitialized: boolean;
className?: string;
}) => {
const cleanupFunction = useRef<(() => void) | null>(null);

Expand Down Expand Up @@ -135,7 +137,7 @@ export const ControlRenderer = <

return component && isControlGroupInitialized ? (
// @ts-expect-error
<ControlPanel<ApiType> Component={component} uuid={uuid} />
<ControlPanel<ApiType> Component={component} uuid={uuid} className={className} />
Heenawter marked this conversation as resolved.
Show resolved Hide resolved
) : // Control group will not display controls until all controls are initialized
null;
};
40 changes: 40 additions & 0 deletions ...plugins/observability_solution/slo/public/pages/slos/components/common/quick_filters.scss
View file
Open in desktop
Copy link
Contributor

@Heenawter Heenawter Sep 17, 2024 •
edited
Loading

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.

Copy link
Contributor Author

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

Copy link
Contributor

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.

Copy link
Contributor

@nreese nreese Sep 23, 2024 •
edited
Loading

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

Copy link
Contributor

@Heenawter Heenawter Sep 23, 2024 •
edited
Loading

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 🙇

Copy link
Contributor

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.

Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
.controlsWrapper {
Heenawter marked this conversation as resolved.
Show resolved Hide resolved
min-height: $euiSize * 4;
}

.optionsList--filterBtn, .euiFilterButton {
height: $euiFormControlHeight !important;
}

.euiFilterGroup {
height: $euiFormControlHeight !important;
border-radius: $euiFormControlBorderRadius !important;
}

.controlPanel {

.euiDualRange, .euiRange {
height: $euiFormControlHeight !important;
}

.controlPanel--label {
height: $euiFormControlHeight !important;
}
}

.dshDashboardViewport-controls {
Heenawter marked this conversation as resolved.
Show resolved Hide resolved
padding-bottom: $euiSizeXS;
}

.controlPanel--labelWrapper > label {
height: $euiFormControlHeight !important;
line-height: 3 !important;
}

.optionsListControl {
height: $euiFormControlHeight !important;
}

.controlFrame__formControlLayout {
block-size: $euiFormControlHeight !important;
}
1 change: 1 addition & 0 deletions .../plugins/observability_solution/slo/public/pages/slos/components/common/quick_filters.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import styled from 'styled-components';
import { Filter } from '@kbn/es-query';
import { isEmpty } from 'lodash';
import { SearchState } from '../../hooks/use_url_search_state';
import './quick_filters.scss';

interface Props {
initialState: SearchState;
Expand Down