Fix missing padding from the page header to the main content

[jennypavlova]

Fixes #140002

Summary

This PR fixes the issue with the missing padding on Inventory, Hosts, and Logs/Stream pages. The issue was introduced with #139524. Using the emotion CSS overrides the defined CSS of the content and the EUI content class is not added.

So as a solution I added an extra class inside the contentProps and defined the styles inside the index.scss. That way the styles will be merged with the component styles and there won't be any override (also they are reused for all pages).

@constancecchen I would like to hear your opinion as well as you may have better ideas as you are more familiar with the EUI components.

[elasticmachine]

Pinging @elastic/infra-monitoring-ui (Team:Infra Monitoring UI)

[formgeist]

@jennypavlova Looking at the visual changes it works, but I don't think it's necessary to create custom styling, the padding should be part of the EuiPageTemplate by default, so I'm wondering why it's not working out of the box. Looking forward to @constancecchen's thoughts on this.

[jennypavlova]

@jennypavlova Looking at the visual changes it works, but I don't think it's necessary to create custom styling, the padding should be part of the EuiPageTemplate by default, so I'm wondering why it's not working out of the box. Looking forward to @constancecchen's thoughts on this.

@formgeist Thank you for the feedback! The padding is part of the component but the custom styles are added with the page template update and they were overriding the padding so I just moved them to a separate class so the styles coming from the eui component (+ the padding) are now there. Maybe we can define the styles for the full height layout under a flag inside contentProps so we can add them when using the eui component whithout the override if it makes sense for other pages as well 🤔.

[cee-chen]

Hey hey! This changeset looks fine, but I'll admit I'm a little confused how the Emotion CSS you converted to Sass is overriding padding. Are you saying that passing a css prop to contentProps overrides (instead of merging, like Emotion should be doing) the padding by set in EUI's css? If so, that's possibly an issue on EUI's end - let me investigate.

[jennypavlova]

Hey hey! This changeset looks fine, but I'll admit I'm a little confused how the Emotion CSS you converted to Sass is overriding padding. Are you saying that passing a css prop to contentProps overrides (instead of merging, like Emotion should be doing) the padding by set in EUI's css? If so, that's possibly an issue on EUI's end - let me investigate.

@constancecchen The issue is that after using the emotion css the class is not applied in the content - docs so the new component will have class css-123 and not css-123-euiPageSection__content-l. I tried to add euiPageSection__content-l as className to contentProps but then it the class will miss the first part (css-123 from the example). That's why I implemented that solution as a workaround. Maybe with Composition in the main eui component, we can achieve what we want 🤔 We can test different possible solutions to avoid defining the styles inside the Kibana app.

[cee-chen]

Hey hey! I'm testing this now on a CodeSandbox with latest EUI and can confirm this is indeed an issue with passing css to contentProps. This definitely needs to get fixed on EUI's end so I'm going to take a look at fixing this and getting in a backport/release to ensure the fix gets in to Kibana by 8.5 FF.

I'll leave it up to y'all as to preference, but that fix should theoretically negate the need for this Sass workaround if you want to wait for it. Totally up to you

[matschaffer]

Extracting the class seems a lot cleaner to me. I'd say go for it unless @elastic/observability-design recommends avoiding it for some reason.

[formgeist]

I'll leave it up to y'all as to preference, but that fix should theoretically negate the need for this Sass workaround if you want to wait for it. Totally up to you

I think it's totally fine to wait for this fix in EUI first 👍 The only thing I'd like to avoid is any possible inconsistencies it might provide across all our apps.

[jennypavlova]

I'll leave it up to y'all as to preference, but that fix should theoretically negate the need for this Sass workaround if you want to wait for it. Totally up to you

@constancecchen Thank you for checking it! I still don't get why setting the emotion css is better, in this case, 🤔( I get the idea why it should be possible and fixed of course 😅 ). I think that it's a good idea to avoid repeating the styles. What do you think about the idea to have them defined inside the content and for example adding a class instead of emotion css repeated 3 times? Or having the styles defined inside the EUI and when it's used to just set an extra prop similar to the paddingSize. So it can look like this:

pageSectionProps={{
        paddingSize: 'none',
        // have a new boolean prop to apply the styles
        fullHeightContet,
        contentProps: {}
}}

Or the styles defined inside the content eui (under a class)

pageSectionProps={{
        paddingSize: 'l',
        // Styles can be defined inside eui under this className
        contentProps: { className: 'fullHeightContet' }
}} 

What do you think about this idea?
Maybe if that's not possible then let's keep the version with the class and Saas styles here if there is nothing against it?

[cee-chen]

@constancecchen Thank you for checking it! I still don't get why setting the emotion css is better, in this case

Oh sorry, it's not strictly better - it's just that when I converted those 3 files/templates to the new Eui/KibanaPageTemplate, they were previously using styled-components to add the same repeated CSS so I just ended up converting it to Emotion instead. So it was basically a 1:1 conversion thing and I didn't make it less DRY than before, at least 😅

TBH, I'm personally ambivalent either way as to whether you use Sass or Emotion; in general I'd say our architecture is trending towards CSS-in-JS over Sass (EUI will eventually be deprecating its usage of Sass and all our exported Sass variables), but I don't know if Kibana will be anytime soon.

If you wanted to DRY this out but still use Emotion, what you could do instead is:

x-pack/plugins/infra/public/page_template.styles.ts // replaces your scss file

import { css } from '@emotion/react';

// This is added `EuiPageSection.contentProps` to facilitate a full height layout whereby the
// inner container will set its own height and be scrollable.
export fullHeightContentStyles = css`
  display: flex;
  flex-direction: column;
  flex: 1 0 auto;
  width: 100%;
  height: 100%;
`;

// in your page template files

import { fullHeightContentStyles } from '../../page_template.styles.ts';
// ...
pageSectionProps={{ contentProps: { css: fullHeightContentStyles } }}

That way you don't repeat your styles but remain in the CSS-in-JS ecosystem.

FYI, the fix for contentProps.css unintentionally overriding EUI's padding CSS will be in main once #140323 merges in (should ideally be sometime next week).

[jennypavlova]

TBH, I'm personally ambivalent either way as to whether you use Sass or Emotion; in general I'd say our architecture is trending towards CSS-in-JS over Sass (EUI will eventually be deprecating its usage of Sass and all our exported Sass variables), but I don't know if Kibana will be anytime soon.

@constancecchen Thank you for the good explanation! I agree let's use emotion then, no preference from my side either. I used your suggestion and pushed the changes. Once your PR (#140323) is merged we can fully test it 🙌

[kibana-ci]

💚 Build Succeeded

• Buildkite Build
• Commit: 4ccc436

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
infra	1011	1012	+1

Async chunks

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

id	before	after	diff
infra	1017.1KB	1016.7KB	-419.0B

History

• 💚 Build #73305 succeeded 82e4c8a
• 💔 Build #73234 failed acb7892
• 💛 Build #72272 was flaky b608db3
• 💚 Build #72150 succeeded 6b1d6f0
• 💔 Build #72122 failed b77b160

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

cc @jennypavlova

[jennypavlova]

I merged the changes locally from the PR (#140323) and it looks good :) The styles are merged correctly now.
So if it's fine with you we can merge this PR together with 140323? @elastic/infra-monitoring-ui @elastic/observability-design

[formgeist]

@jennypavlova Sounds good, feel free to merge