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 v88.1.0 #165047

Merged
merged 32 commits into from
Sep 5, 2023
Merged

Upgrade EUI to v88.1.0 #165047

merged 32 commits into from
Sep 5, 2023

Conversation

1Copenut
Copy link
Contributor

@1Copenut 1Copenut commented Aug 28, 2023

PR change summary

v87.2.0v88.1.0

⚠️ The biggest thing to QA in this PR is several breaking changes to EuiDescriptionList.

Description list columnWidths prop

This PR introduces a new columnWidths prop and removes several Kibana instances of custom CSS overrides to title/description column widths.

The primary motivation behind this is not just to reduce custom CSS, but also because v88.0.0 introduced an underlying CSS change of column description lists to using display: grid CSS. The new prop allows us to support existing description list custom widths while not requiring Kibana developers to understand or write grid CSS (except for 1 or two scenarios around max-width).

⚠️ No user-facing UI around column widths should have regressed as a result of these changes. If they have, please let us know in this PR.

Description list gutter size changes

The prop gutterSize has been renamed to rowGutterSize and the default size is now s instead of m.

The change to s from m means there is an expected smaller gap between list items (see below screenshots):

Current EuiDescriptionList with default rowGutterSize="s"

Prior EuiDescriptionList with default gutterSize="m"

If Kibana teams prefer to keep the previous m gutter for their instances of EuiDescriptionList, you have a couple of options:

  1. Let EUI team know in the PR and we can set usage back to what it was before
  2. Set rowGutterSize="m" yourselves manually

88.1.0

  • Added font.defaultUnits theme token. EUI component font sizes default to rem units - this token allows consumers to configure this to px or em (#7133)
  • Updated EuiDescriptionList with new columnWidths prop (#7146)

Bug fixes

  • Fixed EuiDataGrid's keyboard shortcuts popover display (#7146)

CSS-in-JS conversions

  • Renamed useEuiFontSize()'s measurement option to unit for clarity (#7133)

88.0.0

  • Updated EuiDescriptionList with a new columnGutterSize prop (#7062)

Deprecations

  • Deprecated EuiSuggest. We recommend using EuiSelectable or EuiComboBox instead (#7122)
  • Deprecated EuiControlBar. We recommend using EuiBottomBar instead (#7122)
  • Deprecated EuiColorStops. We recommend copying the component to your application if necessary (#7122)
  • Deprecated EuiNotificationEvent. We recommend copying the component to your application if necessary (#7122)

Breaking changes

  • Renamed EuiDescriptionList's gutterSize prop to rowGutterSize (#7062)
  • EuiDescriptionList's rowGutterSize prop now defaults to a size of s (was previously m) (#7062)

Accessibility

  • Fixed the dark mode colors of inline EuiDescriptionListTitles to meet WCAG color contrast requirements (#7062)

CSS-in-JS conversions

  • Converted EuiKeyPadMenuItem to Emotion; Removed $euiKeyPadMenuSize and $euiKeyPadMenuMarginSize (#7118)

@1Copenut 1Copenut added release_note:skip Skip the PR/issue when compiling release notes EUI backport:skip This commit does not require backporting v8.11.0 labels Aug 28, 2023
@1Copenut 1Copenut self-assigned this Aug 28, 2023
- note: for some reason, `getDOMNode()` is returning an array instead of a single element - appears to be a bug with Enzyme
@@ -96,7 +96,7 @@ export function ShardFailureDescription(props: ShardFailure) {
<EuiFlexItem grow={false}>
<EuiDescriptionList
type="responsiveColumn"
gutterSize="s"
rowGutterSize="s"
Copy link
Member

@cee-chen cee-chen Aug 28, 2023

Choose a reason for hiding this comment

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

We can technically remove these props completely now that rowGutterSize defaults to "s".

Suggested change
rowGutterSize="s"

@kyrspl what was the intention of the breaking EuiDescriptionList gutter size change? Do you want all downstream instances in Kibana to adopt the new default gutter size, or should we update previous usages to set <EuiDescriptionList rowGutterSize="m"> to preserve the old gutter size?

Copy link

@kyrspl kyrspl Aug 29, 2023

Choose a reason for hiding this comment

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

Visual preference - if we think it's going to break a lot of instances let's switch it to m and we can review later.

For clarity, why did you write size="m"?

Copy link
Contributor Author

@1Copenut 1Copenut Aug 29, 2023

Choose a reason for hiding this comment

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

@kyrspl and I discussed this at the beginning of my workday Tuesday (Aug 29). We decided it would be better to update the EuiDescriptionList props rowGutterSize and columnGutterSize to have default m. I'll update the component in EUI, issue a new pull request, then make a minor point release and fold it into this Kibana upgrade.

That way we're keeping the default the same as was prior for row margins, and honoring the local Kibana overrides by passing in s.

/cc @cee-chen

UPDATE:
The whole EUI team discussed this item and the ramifications of reverting a conscious change. We are moving ahead with the breaking change as is, with the default gutter as s. I'll update the Kibana code to handle this breaking change and include screen shots as I can for product teams to review.

@1Copenut 1Copenut marked this pull request as ready for review August 29, 2023 21:49
@1Copenut 1Copenut requested review from a team as code owners August 29, 2023 21:49
@elasticmachine
Copy link
Contributor

Pinging @elastic/eui-team (EUI)

@botelastic botelastic bot added the Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability label Aug 29, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

Copy link
Contributor

@opauloh opauloh left a comment

Choose a reason for hiding this comment

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

Cloud Security lgtm

@@ -26,7 +26,7 @@ Array [
data-type="row"
>
<dt
class="euiDescriptionList__title emotion-euiDescriptionList__title-row-m-normal"
class="euiDescriptionList__title emotion-euiDescriptionList__title-row-normal-s"
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be a direct consequence of the changed default size, right? Do we want to specify size m explicitly in the corresponding React element call so it doesn't change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@weltenwort Correct, the snapshots updated because the default size was changed to s. This was a conscious decision by EUI to reduce the margin between rows on EuiDescriptionList. If there are layouts you want to retain the larger margin, they can be overrode by passing rowGutterSize="m" into the component.

@cee-chen cee-chen changed the title Upgrade EUI to v88.0.0 Upgrade EUI to v88.1.0 Aug 31, 2023
@kibanamachine kibanamachine requested a review from a team as a code owner September 1, 2023 00:34
Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

Tested ML changes and LGTM

@cee-chen
Copy link
Member

cee-chen commented Sep 1, 2023

Hey all - just want to give a heads up, I'm not sure what's going on with the failing serverless tests and I'm pretty sure they're not related to EUI. We'll continue merging main in hopes that they get fixed in main, and talk to KibanaOps if it's not looking to improve, but until then, please ignore any serverless CI failures and focus on EuiDescriptionList as the main item to QA in this PR.

Copy link
Contributor

@nikitaindik nikitaindik left a comment

Choose a reason for hiding this comment

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

Reviewed the Security Solution Rules Management screens. Found a couple display issues. Discussed these with @cee-chen. Fixes for these were merged in with this commit.

Now Security Solution Rules Management part LGTM.

Copy link
Contributor

@tomsonpl tomsonpl left a comment

Choose a reason for hiding this comment

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

LGTM, Management views look ok to me :) But let's wait for a second pair of eyes too 👍

Copy link
Contributor

@angorayc angorayc 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, LGTM, thank you!

Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

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

monitoring plugin changes LGTM

@cee-chen
Copy link
Member

cee-chen commented Sep 5, 2023

@elastic/security-threat-hunting-investigations @elastic/kibana-visualizations @elastic/kibana-presentation @elastic/security-defend-workflows - last call for reviews/QA checks, we'll be asking KibanaOps to admin merge by EOD.

As always, however, you're welcome to ping us later for help if you run into issues!

@kibana-ci
Copy link
Collaborator

kibana-ci commented Sep 5, 2023

💔 Build Failed

Failed CI Steps

Test Failures

  • [job] [logs] Serverless Security Explore Cypress Tests #2 / Entity Analytics Dashboard in Serverless "before each" hook for "should display a splash screen when visited with Security essentials PLI " "before each" hook for "should display a splash screen when visited with Security essentials PLI "
  • [job] [logs] Explore - Security Solution Cypress Tests #4 / Entity Analytics Dashboard With host risk data filters by risk classification filters by risk classification
  • [job] [logs] Defend Workflows Endpoint Cypress Tests #6 / Response console document signing "after all" hook for "should fail if data tampered" "after all" hook for "should fail if data tampered"
  • [job] [logs] Defend Workflows Endpoint Cypress Tests #6 / Response console document signing "before all" hook for "should fail if data tampered" "before all" hook for "should fail if data tampered"
  • [job] [logs] Serverless Security Examples Tests / serverless examples UI search examples Search example with bfetch should have an other bucket

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
cases 668 670 +2
core 481 483 +2
fleet 939 941 +2
kibanaReact 325 327 +2
kibanaUtils 181 183 +2
security 500 502 +2
securitySolution 4499 4502 +3
total +15

Async chunks

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

id before after diff
canvas 1.0MB 1.0MB -33.0B
cases 423.1KB 426.8KB +3.6KB
core 146.7KB 150.3KB +3.6KB
data 54.8KB 54.1KB -732.0B
dataVisualizer 606.1KB 605.5KB -646.0B
fleet 1.2MB 1.2MB +3.6KB
infra 2.0MB 2.0MB +1.0B
kibanaReact 209.4KB 213.0KB +3.6KB
kibanaUtils 60.8KB 64.4KB +3.6KB
ml 3.5MB 3.5MB -1.5KB
security 568.0KB 571.6KB +3.6KB
securitySolution 12.6MB 12.6MB +14.4KB
sessionView 392.0KB 391.9KB -28.0B
synthetics 913.1KB 913.0KB -43.0B
triggersActionsUi 1.4MB 1.4MB -269.0B
visTypeVega 1.8MB 1.8MB +1.0B
total +32.8KB

Page load bundle

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

id before after diff
kbnUiSharedDeps-css 284.1KB 279.4KB -4.7KB
kbnUiSharedDeps-npmDll 6.2MB 6.2MB +3.5KB
kbnUiSharedDeps-srcJs 2.2MB 2.2MB +1.0B
total -1.2KB

History

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

cc @1Copenut

@jbudz jbudz merged commit 3cb13fb into elastic:main Sep 5, 2023
35 of 48 checks passed
ashokaditya added a commit that referenced this pull request Sep 7, 2023
…ails flyout (#165937)

## Summary

This PR fixes a UX bug that was a result of EUI upgrade where the
description list contents were misaligned. The `EuiDescriptionList`
component should take a set of column widths and column and row gutter
sizes to allow for adequate spacing between text elements.

**before**
![Screenshot 2023-09-07 at 10 16 11
AM](https://github.com/elastic/kibana/assets/1849116/80984552-b611-40ea-98c5-945aa3b179d7)

**with this fix**
![Screenshot 2023-09-07 at 10 15 26
AM](https://github.com/elastic/kibana/assets/1849116/fe1fd47f-8905-4808-bd14-f891fc345a2b)


see /pull/165047

### Checklist
- [ ] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [x] Any UI touched in this PR is usable by keyboard only (learn more
about [keyboard accessibility](https://webaim.org/techniques/keyboard/))
- [x] Any UI touched in this PR does not create any new axe failures
(run axe in browser:
[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),
[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))
- [x] This renders correctly on smaller devices using a responsive
layout. (You can test this [in your
browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))
- [x] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)
bryce-b pushed a commit to bryce-b/kibana that referenced this pull request Sep 19, 2023
## PR change summary

`v87.2.0`⏩`v88.1.0`

⚠️ The biggest thing to QA in this PR is several **breaking changes** to
`EuiDescriptionList`.

### Description list `columnWidths` prop

This PR introduces a new `columnWidths` prop and removes several Kibana
instances of custom CSS overrides to title/description column widths.

The primary motivation behind this is not just to reduce custom CSS, but
also because v88.0.0 introduced an underlying CSS change of `column`
description lists to using [`display: grid`
CSS](https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_grid_layout).
The new prop allows us to support existing description list custom
widths while not requiring Kibana developers to understand or write grid
CSS (except for 1 or two scenarios around `max-width`).

⚠️ **No user-facing UI around column widths should have regressed as a
result of these changes. If they have, please let us know in this PR.**

### Description list gutter size changes

The prop `gutterSize` has been renamed to `rowGutterSize` and the
default size is now `s` instead of `m`.

The change to `s` from `m` means there is an **expected** smaller gap
between list items (see below screenshots):

**Current `EuiDescriptionList` with default `rowGutterSize="s"`**
<img width="753" alt=""
src="https://github.com/elastic/kibana/assets/934879/c17aef28-ed3b-40c4-84c3-396e788b13bb">

**Prior `EuiDescriptionList` with default `gutterSize="m"`**
<img width="721" alt=""
src="https://github.com/elastic/kibana/assets/934879/84d5f5a2-8fa6-4f99-9dc0-73fd143aa1e1">

If Kibana teams prefer to keep the previous `m` gutter for their
instances of `EuiDescriptionList`, you have a couple of options:

1. Let EUI team know in the PR and we can set usage back to what it was
before
2. Set `rowGutterSize="m"` yourselves manually

---

## [`88.1.0`](https://github.com/elastic/eui/tree/v88.1.0)

- Added `font.defaultUnits` theme token. EUI component font sizes
default to `rem` units - this token allows consumers to configure this
to `px` or `em` ([elastic#7133](elastic/eui#7133))
- Updated `EuiDescriptionList` with new `columnWidths` prop
([elastic#7146](elastic/eui#7146))

**Bug fixes**

- Fixed `EuiDataGrid`'s keyboard shortcuts popover display
([elastic#7146](elastic/eui#7146))

**CSS-in-JS conversions**

- Renamed `useEuiFontSize()`'s `measurement` option to `unit` for
clarity ([elastic#7133](elastic/eui#7133))

## [`88.0.0`](https://github.com/elastic/eui/tree/v88.0.0)

- Updated `EuiDescriptionList` with a new `columnGutterSize` prop
([elastic#7062](elastic/eui#7062))

**Deprecations**

- Deprecated `EuiSuggest`. We recommend using `EuiSelectable` or
`EuiComboBox` instead
([elastic#7122](elastic/eui#7122))
- Deprecated `EuiControlBar`. We recommend using `EuiBottomBar` instead
([elastic#7122](elastic/eui#7122))
- Deprecated `EuiColorStops`. We recommend copying the component to your
application if necessary
([elastic#7122](elastic/eui#7122))
- Deprecated `EuiNotificationEvent`. We recommend copying the component
to your application if necessary
([elastic#7122](elastic/eui#7122))

**Breaking changes**

- Renamed `EuiDescriptionList`'s `gutterSize` prop to `rowGutterSize`
([elastic#7062](elastic/eui#7062))
- `EuiDescriptionList`'s `rowGutterSize` prop now defaults to a size of
`s` (was previously `m`)
([elastic#7062](elastic/eui#7062))

**Accessibility**

- Fixed the dark mode colors of inline `EuiDescriptionListTitle`s to
meet WCAG color contrast requirements
([elastic#7062](elastic/eui#7062))

**CSS-in-JS conversions**

- Converted `EuiKeyPadMenuItem` to Emotion; Removed `$euiKeyPadMenuSize`
and `$euiKeyPadMenuMarginSize`
([elastic#7118](elastic/eui#7118))

---------

Co-authored-by: Cee Chen <constance.chen@elastic.co>
Co-authored-by: Cee Chen <549407+cee-chen@users.noreply.github.com>
Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Nikita Indik <nikita.indik@elastic.co>
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 EUI release_note:skip Skip the PR/issue when compiling release notes Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v8.11.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.