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

[SharedUX] Removing TODOs from KibanaPageTemplate #141043

Merged
merged 12 commits into from
Sep 22, 2022

Conversation

majagrubic
Copy link
Contributor

@majagrubic majagrubic commented Sep 20, 2022

Summary

Fixes: #140118

This PR addresses some of the TODOs that were left in the code when migrating from kibana_react to a shared-ux package. Some changes:

  1. Getting rid of with_solution_nav scss file and rewriting code in emotion
  2. Getting offset from kibanaChrome in page_template_inner
  3. Beautifying props

Checklist

Delete any items that are not applicable to this PR.

- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
- [ ] Documentation was added for features that require explanation or tutorials

  • Unit or functional tests were updated or added to match the most common scenarios
  • Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
  • Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
    - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
  • This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
  • This was checked for cross-browser compatibility

For maintainers

@majagrubic majagrubic added release_note:skip Skip the PR/issue when compiling release notes v8.5.0 labels Sep 20, 2022
@majagrubic majagrubic marked this pull request as ready for review September 21, 2022 06:57
@majagrubic majagrubic requested a review from a team September 21, 2022 06:57
Copy link
Contributor

@rshen91 rshen91 left a comment

Choose a reason for hiding this comment

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

These changes look great to me - tested locally


export const WithSolutionNavStyles = (euiTheme: EuiThemeComputed<{}>) => {
return css`
flex: 0 1 0%;
Copy link
Contributor

Choose a reason for hiding this comment

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

woohoo!!!

@majagrubic majagrubic added v8.6.0 and removed v8.5.0 labels Sep 21, 2022
Copy link
Contributor

@clintandrewhall clintandrewhall left a comment

Choose a reason for hiding this comment

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

Thanks for doing this!

@@ -0,0 +1,20 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

I need to remember to do this. I always forget...!

import { css } from '@emotion/react';
import { euiCanAnimate, EuiThemeComputed } from '@elastic/eui';

export const WithSolutionNavStyles = (euiTheme: EuiThemeComputed<{}>) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I don't think this should start with a capital... it's not a React component, or a class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to change it, but would prefer if we stayed consistent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will merge this now, but lmk if you think it needs to be changed.

Maja Grubic and others added 2 commits September 22, 2022 13:23
…_inner.tsx

Co-authored-by: Clint Andrew Hall <clint@clintandrewhall.com>
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
dashboard 398 394 -4
discover 533 529 -4
enterpriseSearch 1712 1708 -4
esUiShared 220 216 -4
graph 289 285 -4
home 179 175 -4
infra 1011 1007 -4
kibanaOverview 105 101 -4
lens 909 905 -4
management 88 84 -4
maps 994 990 -4
observability 471 467 -4
security 465 461 -4
securitySolution 3142 3138 -4
visualizations 321 317 -4
total -60

Async chunks

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

id before after diff
dashboard 387.5KB 387.6KB +90.0B
discover 477.4KB 476.5KB -915.0B
enterpriseSearch 1.7MB 1.7MB +70.0B
graph 450.1KB 450.2KB +95.0B
home 147.0KB 147.0KB +95.0B
infra 1.0MB 1.0MB +75.0B
kibanaOverview 46.2KB 46.3KB +104.0B
lens 1.2MB 1.2MB -771.0B
management 30.4KB 30.5KB +100.0B
maps 2.6MB 2.6MB +95.0B
observability 522.5KB 522.5KB +75.0B
security 536.1KB 536.1KB +75.0B
securitySolution 6.5MB 6.5MB +76.0B
visualizations 218.0KB 218.1KB +95.0B
total -641.0B

Page load bundle

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

id before after diff
esUiShared 152.7KB 152.8KB +100.0B

History

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

@majagrubic majagrubic merged commit aea8a9e into elastic:main Sep 22, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Sep 22, 2022
@majagrubic majagrubic deleted the eui-pagetemplate branch September 22, 2022 12:39
jloleysens added a commit to jloleysens/kibana that referenced this pull request Sep 22, 2022
* main: (33 commits)
  Fix Next and Previous button on step screenshot carousel. (elastic#141422)
  [journeys] add a short delay after loading data (elastic#141437)
  skip failing test suite (elastic#140797)
  skip failing test suite (elastic#138776)
  Documents saved objects bulk delete API (elastic#141164)
  [Response Ops][Alerting] Defining default action params on connector type (elastic#141226)
  [ML] Explain Log Rate Spikes: Adds jest tests for query_utils/buildBaseFilterCriteria. (elastic#141213)
  [APM] Fix search bar suggestions (elastic#141101)
  close popover on click (elastic#141272)
  [Fleet] Update to use savedObjects bulkDelete for package policy delete (elastic#141276)
  [Security Solution][Endpoint][Response Actions] Do fuzzy search on given usernames for Actions Log (elastic#141239)
  [Graph] Fix guidance panel appearing for a moment when saving Graph (elastic#141228)
  [Fleet] Add experimental data stream features support to simplified package policy API (elastic#141288)
  Shameless copy of the retryTransientEsErrors from fleet (elastic#141246)
  [Security Solution][Fix]-Issue with disabled dataProvider (elastic#140735)
  Changing triggers actions ui routes to internal (elastic#141149)
  skip flaky test suit elastic#141356
  [SharedUX] Removing TODOs from KibanaPageTemplate (elastic#141043)
  [dashboard controls] skip failing test on cloud (elastic#141291)
  [Synthetics] unskip edit_monitor api integration tests (elastic#141277)
  ...
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 v8.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[meta][shared-ux] Follow up to EUI v63.x upgrade
5 participants