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.

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

[Embedded Console] Introduce kbnSolutionNavOffset CSS variable #175348

Merged

Conversation

TattdCodeMonkey
Copy link
Contributor

@TattdCodeMonkey TattdCodeMonkey commented Jan 23, 2024

Summary

This PR updates the Kibana SolutionNav component to set a global CSS variable --kbnSolutionNavOffset with it's current width. This is modeled off of the EUI CSS variable --euiCollapsibleNavOffset that can be used in Serverless to get the current width of the side nav.

This will be used by the embedded dev console to ensure it is not rendered over the page side navigation, at least for pages that use the KibanaPageTemplate & SolutionNav.

Testing

The easiest way to verify this change is to check the values of --kbnSolutionNavOffset in the browser console, which can be done with the following command:
getComputedStyle(document.documentElement).getPropertyValue('--kbnSolutionNavOffset')

You can check this with the solution nav open and closed or on pages without a solution nav at all (/app/home for example)

@TattdCodeMonkey TattdCodeMonkey added release_note:skip Skip the PR/issue when compiling release notes Team:Search labels Jan 23, 2024
@TattdCodeMonkey TattdCodeMonkey requested review from a team as code owners January 23, 2024 20:47
Updated the SolutionNav component to set a global CSS variable,
--kbnSolutionNavOffset, with the current width of the solution nav. This
will allow components in Kibana to offset based on the current size of
the solution nav if its rendered on the page.
Updated the embedded console to check the current chrome style and use
the correct CSS variable for the side nav offset.
@TattdCodeMonkey TattdCodeMonkey force-pushed the rnorris/kbn-solution-nav-offset branch from dbc933f to 004ff2b Compare January 23, 2024 21:33
Copy link
Contributor

@yuliacech yuliacech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally, changes LGTM 👍

@TattdCodeMonkey TattdCodeMonkey requested a review from a team as a code owner January 24, 2024 15:17
useEffect(() => {
setGlobalCSSVariables({
'--kbnSolutionNavOffset': navWidth,
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this should be cleaned in the destroy callback?

Copy link
Contributor Author

@TattdCodeMonkey TattdCodeMonkey Jan 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had that initially, but it didn't seem to change anything so I removed it as superfluous.

Also the eui code I modeled this off of does not have a destroy callback either.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it could be a problem when this variable is used with a fallback for the case when solution nav is not rendered, e.g.:

var(--kbnSolutionNavOffset, 0);

if (isMediumBreakpoint) {
return isSideNavOpenOnMobile || !canBeCollapsed
? `${FLYOUT_SIZE}px`
: euiThemeVars.euiSizeXXL;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like there is no way (or no straightforward way) to connect these widths with the widths that are actually set for the sidebar. Maybe it is worth at least adding a comment in the CSS where these widths are set, that this logic might also need an update in case the CSS is updated

like here

minWidth: isSidebarShrunk ? euiTheme.size.xxl : undefined,

or here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe it is worth at least connecting them with css variable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Dosant I added variables for the sizes and added a comment about it also being set in the tsx file.

I don't think there is way to read the variables from the scss file in the tsx file at compile time without configuration changes. But if I'm wrong about that let me know.

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Async chunks

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

id before after diff
canvas 1013.8KB 1014.1KB +262.0B
console 424.2KB 424.7KB +483.0B
dashboard 382.3KB 382.5KB +259.0B
enterpriseSearch 2.7MB 2.7MB +259.0B
eventAnnotationListing 196.9KB 197.2KB +254.0B
filesManagement 89.9KB 90.2KB +261.0B
graph 387.1KB 387.4KB +261.0B
home 137.5KB 137.7KB +261.0B
indexManagement 587.0KB 587.3KB +262.0B
infra 1.3MB 1.3MB +259.0B
kibanaOverview 49.9KB 50.1KB +259.0B
management 77.5KB 77.8KB +261.0B
maps 2.9MB 2.9MB +262.0B
metricsDataAccess 85.3KB 85.6KB +262.0B
ml 3.6MB 3.6MB +262.0B
observabilityShared 36.4KB 36.7KB +261.0B
osquery 1.0MB 1.0MB +262.0B
security 579.7KB 580.0KB +259.0B
securitySolution 11.1MB 11.1MB +518.0B
securitySolutionEss 44.5KB 44.7KB +262.0B
securitySolutionServerless 220.5KB 220.8KB +259.0B
spaces 174.9KB 175.2KB +261.0B
visualizations 270.4KB 270.6KB +259.0B
total +6.3KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
esUiShared 155.8KB 156.1KB +262.0B

History

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

@TattdCodeMonkey TattdCodeMonkey merged commit 98cb0c2 into elastic:main Jan 24, 2024
19 checks passed
@TattdCodeMonkey TattdCodeMonkey deleted the rnorris/kbn-solution-nav-offset branch January 24, 2024 19:22
@kibanamachine kibanamachine added v8.13.0 backport:skip This commit does not require backporting labels Jan 24, 2024
jloleysens added a commit that referenced this pull request Jan 25, 2024
* main: (520 commits)
  Update Kibana code editor dependencies (#171720)
  [SLOs] Hide view in app in slo alerts table in slo details page (#175441)
  [api-docs] 2024-01-25 Daily api_docs build (#175502)
  [DOCS] Add buildkite links to doc preview comments (#175463)
  skip flaky suite (#175443)
  [Security Solution][Timeline] refactor timeline modal save timeline button (#175343)
  [RAM] Stack Management::Rules loses user selections when navigating back (#174954)
  [Security Solution][Timeline] refactor timeline modal attach to case button (#175163)
  Upgrade EUI to v92.1.1 (#174955)
  [Fleet]: Beta label is shown inconsistently while selecting proxy under Fleet settings. (#170634)
  [Cloud Security] Rules Combo Box filters Custom component (#175175)
  skip flaky suite (#175407)
  [Security Solution][Timeline] refactor timeline modal open timeline button (#175335)
  [Embedded Console] Introduce kbnSolutionNavOffset CSS variable (#175348)
  [Console] disable access to embedded console without dev tools capability (#175321)
  fix(x-pack/reporting): use FIPS-compliant ID generator `uuidv4` in Reporting plugin (#174809)
  [Security Solution] Data quality dashboard persistence (#173185)
  [RAM][Observability] Add alert fields table to Observability flyout (#174685)
  test: add missing await for connector table disappearance (#175430)
  [RAM][Maintenance Window] Fix maintenance window FE types and transforms  (#173888)
  ...
CoenWarmer pushed a commit to CoenWarmer/kibana that referenced this pull request Feb 15, 2024
…ic#175348)

## Summary

This PR updates the Kibana `SolutionNav` component to set a global CSS
variable `--kbnSolutionNavOffset` with it's current width. This is
modeled off of the EUI CSS variable `--euiCollapsibleNavOffset` that can
be used in Serverless to get the current width of the side nav.

This will be used by the embedded dev console to ensure it is not
rendered over the page side navigation, at least for pages that use the
`KibanaPageTemplate` & `SolutionNav`.

### Testing

The easiest way to verify this change is to check the values of
`--kbnSolutionNavOffset` in the browser console, which can be done with
the following command:

`getComputedStyle(document.documentElement).getPropertyValue('--kbnSolutionNavOffset')`

You can check this with the solution nav open and closed or on pages
without a solution nav at all (`/app/home` for example)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:Search v8.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants