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

Upgrade EUI to v54.0.0 #129653

Merged
merged 18 commits into from
Apr 12, 2022
Merged

Upgrade EUI to v54.0.0 #129653

merged 18 commits into from
Apr 12, 2022

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented Apr 6, 2022

Summary

eui@53.0.1eui@54.0.0

54.0.0

  • EuiDataGrid now allows limiting the number of visible cell actions with a new columns.visibleCellActions prop (defaults to 2). All additional actions will be shown in the cell expansion popover. (#5675)
  • Added a new gridStyle.rowClasses API to EuiDataGrid, which allows adding custom classes/styling to specific row indices, primarily for the purpose of highlighting rows (#5732)
  • Added alert icon indicator and aria-invalid when the following form controls are isInvalid: EuiFieldNumber, EuiFieldPassword, EuiFieldText, EuiSelect, EuiSuperSelect, EuiFieldSearch, EuiColorPicker (#5738)
  • Added isInvalid prop to EuiFormControlLayout to render the alert icon (#5738)
  • Added isDropdown prop to EuiFormControlLayout to create and control an arrowDown icon (#5738)
  • Added color as to EuiFormControlLayout's icon object (#5738)

Bug fixes

  • Fixed EuiSuperSelect border-radius with append or prepend (#5738)
  • Fixed EuiSuperSelect not respecting readOnly (#5738)
  • Fixed EuiDataGrid cell focus sometimes not being restored for keyboard users when cell expansion popovers were closed (#5761)

Breaking changes

  • Removed the closePopover() callback passed to EuiDataGrid's cellActions render functions. Use closeCellPopover() passed by EuiDataGrid's ref prop instead. (#5734)

CSS-in-JS conversions

  • Converted EuiAvatar to CSS-in-JS styling (#5670)

@cee-chen cee-chen added release_note:skip Skip the PR/issue when compiling release notes EUI backport:skip This commit does not require backporting v8.3.0 labels Apr 6, 2022
@@ -212,6 +212,7 @@ const StatefulEventsViewerComponent: React.FC<Props> = ({
dataProviders,
dataViewId,
defaultCellActions,
visibleCellActions: 3,
Copy link
Member Author

Choose a reason for hiding this comment

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

@MikePaquette (pinging you because you posted in elastic/eui#5675 (comment))

I'm not overly familiar with Security's usage of EuiDataGrid - can you confirm that this component (Events Viewer?) is the correct/only component that needs 3 visible cell actions instead of the default 2?

Copy link
Contributor

Choose a reason for hiding this comment

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

From a slack thread:

imo would be safer to just pass a hard coded 3 here https://github.com/elastic/kibana/blob/main/x-pack/plugins/timelines/public/components/t_grid/body/index.tsx#L819-#L839 instead of as a prop from the different places it’s used

Copy link
Contributor

Choose a reason for hiding this comment

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

@MikePaquette (pinging you because you posted in elastic/eui#5675 (comment))

I'm not overly familiar with Security's usage of EuiDataGrid - can you confirm that this component (Events Viewer?) is the correct/only component that needs 3 visible cell actions instead of the default 2?

@constancecchen yes, this is the only component that needs 3 visible cell actions instead of the default 2

- EUI added 2 `.euiPopoverFooter`s for overflowing cell actions, and Security's CSS to hide the first 2 cell actions (replaced by their own custom cell actions) was unintentionally affecting other actions
@cee-chen cee-chen marked this pull request as ready for review April 7, 2022 18:26
@cee-chen cee-chen requested review from a team as code owners April 7, 2022 18:26
Copy link
Member

@sabarasaba sabarasaba left a comment

Choose a reason for hiding this comment

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

@elastic/platform-deployment-management changes lgtm

@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label Apr 11, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui (Team:apm)

@@ -720,6 +728,7 @@ export const BodyComponent = React.memo<StatefulBodyProps>(
columnHeaders,
data,
defaultCellActions,
visibleCellActions,
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of passing this down as a prop from all the places it's used, wouldn't it be safer to just hard code the 3 in this component?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah that would be fine! I wasn't sure what all plugins used this TGrid component and if future usages might want to customize visibleCellActions, so was trying to play it safe. If you're confident all future usages will want a constant max of 3 visible cell actions I'm good with hard-coding it.

Copy link
Member Author

Choose a reason for hiding this comment

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

fc93474 @kqualters-elastic @andrew-goldstein. Here's the relevant line where 3 is hard-coded:

Comment on lines 148 to 150
<Insertion
cache={
Object {
Copy link
Contributor

Choose a reason for hiding this comment

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

😬 these Spaces snapshot test changes aren't great --
The tests themselves are ancient and I found that I could make a few small changes to avoid these gigantic snapshots.
If you're OK with it, I can push these changes to your branch. Let me know!

Copy link
Contributor

Choose a reason for hiding this comment

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

We spoke offline and I pushed the changes here: 3ddd8bd

Copy link
Contributor

@Heenawter Heenawter left a comment

Choose a reason for hiding this comment

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

Code review - primarily just adding new classes to Storybook components, plus some other minor restructuring of Color Manager Storybook. Presentation changes LGTM 👍

Copy link
Contributor

@andrew-goldstein andrew-goldstein left a comment

Choose a reason for hiding this comment

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

Thanks @constancecchen for upgrading the Security Solution to use EuiDataGrid's new visibleCellActions option, such that there's no change in the behavior of the app 🙏

LGTM

Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

DataDiscover.team code owned code LGTM, tested in Cloud, works as expected, I'm exited about that
10kb less when viewing page load panel
More than 10kb less in Discover's page load bundle 🥳 ... just wondering, how comes?

@cee-chen
Copy link
Member Author

More than 10kb less in Discover's page load bundle 🥳 ... just wondering, how comes?

Honestly I'm not sure 🙈 I'm not seeing anything in the diffs that would significantly account for that, so my best guess is maybe Emotion dependency upgrades?

Copy link
Contributor

@kevinlog kevinlog left a comment

Choose a reason for hiding this comment

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

I see that @andrew-goldstein and @kqualters-elastic have also reviewed with checked out code in Security solution. Onboarding-Lifecycle just owns a snapshot update used for testing, which LGTM! (tests would fail otherwise).

Copy link
Contributor

@mbondyra mbondyra left a comment

Choose a reason for hiding this comment

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

LGTM 👌🏼

@kertal
Copy link
Member

kertal commented Apr 12, 2022

More than 10kb less in Discover's page load bundle 🥳 ... just wondering, how comes?

Honestly I'm not sure 🙈 I'm not seeing anything in the diffs that would significantly account for that, so my best guess is maybe Emotion dependency upgrades?

Webpack_Bundle_Analyzer__11_Apr_2022_at_20_13__und_Bildschirmaufnahme_und_Webpack_Bundle_Analyzer__11_Apr_2022_at_17_02_

You're right, styles.js is no longer in the bundle twice!

Copy link
Member

@tsullivan tsullivan left a comment

Choose a reason for hiding this comment

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

app services changes LGTM

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
dataViewEditor 176 177 +1
discover 447 445 -2
maps 886 887 +1
spaces 287 288 +1
total +1

Async chunks

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

id before after diff
apm 2.8MB 2.8MB -18.0B
dataViewEditor 175.8KB 176.3KB +534.0B
discover 407.0KB 407.0KB -6.0B
lens 1.0MB 1.0MB +79.0B
maps 2.7MB 2.7MB +533.0B
securitySolution 4.8MB 4.8MB +4.0B
spaces 290.4KB 290.9KB +533.0B
visTypeTable 15.7KB 15.8KB +69.0B
visTypeTimeseries 470.6KB 470.7KB +92.0B
total +1.8KB

Page load bundle

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

id before after diff
core 319.1KB 320.0KB +943.0B
discover 50.7KB 40.5KB -10.2KB
kbnUiSharedDeps-css 595.5KB 594.5KB -1.0KB
kbnUiSharedDeps-npmDll 4.8MB 4.8MB +4.0KB
timelines 286.8KB 286.9KB +146.0B
total -6.2KB

History

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

Copy link
Contributor

@jportner jportner left a comment

Choose a reason for hiding this comment

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

🎉

@cee-chen
Copy link
Member Author

Thanks everyone!!

Picture of dog holding another dog back with caption 'Teamwork makes the dreamwork'

@cee-chen cee-chen merged commit 0955953 into elastic:main Apr 12, 2022
@cee-chen cee-chen deleted the eui-54.0.0 branch April 12, 2022 18:00
jloleysens added a commit to jloleysens/kibana that referenced this pull request Apr 13, 2022
…disable-server-side

* 'main' of github.com:elastic/kibana: (35 commits)
  [Uptime] remove latency limit warnings when using monitor management (elastic#129597)
  [Security Solution] [ReponseOps] Executes Cases Cypress test when there is a change on cases plugin (elastic#129992)
  Paramaterized Discover tests (elastic#129684)
  [Security Solution][Investigations] - Minor bug fixes (elastic#130054)
  [DOCS} Adds technical preview to Lens annotations (elastic#130058)
  [Security solution] [Endpoint] Revisit blocklist wrong labels (elastic#128773)
  [Security Solutions] Adds API docs for value lists (elastic#129962)
  [CI] Move jest tests to spot instances, and fix spot retries in PRs (elastic#130045)
  chore(NA): upgrades rules_node_js to v5.4.0 (elastic#130051)
  [SecuritySolution] Remove the cell hovers actions for agent status (elastic#130042)
  Upgrade RxJS to 7 (elastic#129087)
  [SecuritySolution] Clean up CaseContext (elastic#130036)
  Revert "chore(NA): upgrades rules_node_js to v5.4.0 (elastic#130021)"
  Use RuleDataReader to query for threshold signal history (elastic#129763)
  Remove securityRulesCancelEnabled setting and set shorter default timeouts (elastic#129769)
  Upgrade EUI to v54.0.0 (elastic#129653)
  [Security Solution] More Ransomware exceptionable fields (elastic#130039)
  Add e2e for the apm integration policy form (elastic#129860)
  chore(NA): upgrades rules_node_js to v5.4.0 (elastic#130021)
  [ML] Fix Single Metric Viewer chart failing to load if no points during calendar event (elastic#130000)
  ...

# Conflicts:
#	x-pack/plugins/screenshotting/server/screenshots/index.test.ts
jloleysens added a commit to jloleysens/kibana that referenced this pull request Apr 13, 2022
…rint-media-attempt-2

* 'main' of github.com:elastic/kibana: (75 commits)
  [Lens] Hide disabled toolbar entries (elastic#129994)
  Fix explore tables don't display data when a global filter is applied (elastic#130024)
  [Console] Add option to disable keyboard shortcuts (elastic#128887)
  [Discover] Update refreshOnClick flaky test (elastic#130001)
  [Uptime] remove latency limit warnings when using monitor management (elastic#129597)
  [Security Solution] [ReponseOps] Executes Cases Cypress test when there is a change on cases plugin (elastic#129992)
  Paramaterized Discover tests (elastic#129684)
  [Security Solution][Investigations] - Minor bug fixes (elastic#130054)
  [DOCS} Adds technical preview to Lens annotations (elastic#130058)
  [Security solution] [Endpoint] Revisit blocklist wrong labels (elastic#128773)
  [Security Solutions] Adds API docs for value lists (elastic#129962)
  [CI] Move jest tests to spot instances, and fix spot retries in PRs (elastic#130045)
  chore(NA): upgrades rules_node_js to v5.4.0 (elastic#130051)
  [SecuritySolution] Remove the cell hovers actions for agent status (elastic#130042)
  Upgrade RxJS to 7 (elastic#129087)
  [SecuritySolution] Clean up CaseContext (elastic#130036)
  Revert "chore(NA): upgrades rules_node_js to v5.4.0 (elastic#130021)"
  Use RuleDataReader to query for threshold signal history (elastic#129763)
  Remove securityRulesCancelEnabled setting and set shorter default timeouts (elastic#129769)
  Upgrade EUI to v54.0.0 (elastic#129653)
  ...

# Conflicts:
#	x-pack/plugins/screenshotting/server/formats/pdf/index.ts
@tylersmalley tylersmalley added ci:cloud-deploy Create or update a Cloud deployment and removed ci:deploy-cloud labels Aug 17, 2022
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 ci:cloud-deploy Create or update a Cloud deployment EUI release_note:skip Skip the PR/issue when compiling release notes Team:APM All issues that need APM UI Team support v8.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.