-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Upgrade EUI to v88.1.0 #165047
Conversation
- 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" |
There was a problem hiding this comment.
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"
.
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?
There was a problem hiding this comment.
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"
?
There was a problem hiding this comment.
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.
Pinging @elastic/eui-team (EUI) |
src/plugins/data/public/shard_failure_modal/shard_failure_description.tsx
Show resolved
Hide resolved
Pinging @elastic/uptime (Team:uptime) |
There was a problem hiding this 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" |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
- goal is to reduce churn whenever eui theme updates, and also prefer RTL over enzyme
…-ref HEAD~1..HEAD --fix'
There was a problem hiding this 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
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this 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 👍
There was a problem hiding this 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!
There was a problem hiding this 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
@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! |
💔 Build FailedFailed CI Steps
Test Failures
Metrics [docs]Module Count
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: cc @1Copenut |
…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)
## 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>
PR change summary
v87.2.0
⏩v88.1.0
EuiDescriptionList
.Description list
columnWidths
propThis 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 usingdisplay: 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 aroundmax-width
).Description list gutter size changes
The prop
gutterSize
has been renamed torowGutterSize
and the default size is nows
instead ofm
.The change to
s
fromm
means there is an expected smaller gap between list items (see below screenshots):Current
EuiDescriptionList
with defaultrowGutterSize="s"
Prior
EuiDescriptionList
with defaultgutterSize="m"
If Kibana teams prefer to keep the previous
m
gutter for their instances ofEuiDescriptionList
, you have a couple of options:rowGutterSize="m"
yourselves manually88.1.0
font.defaultUnits
theme token. EUI component font sizes default torem
units - this token allows consumers to configure this topx
orem
(#7133)EuiDescriptionList
with newcolumnWidths
prop (#7146)Bug fixes
EuiDataGrid
's keyboard shortcuts popover display (#7146)CSS-in-JS conversions
useEuiFontSize()
'smeasurement
option tounit
for clarity (#7133)88.0.0
EuiDescriptionList
with a newcolumnGutterSize
prop (#7062)Deprecations
EuiSuggest
. We recommend usingEuiSelectable
orEuiComboBox
instead (#7122)EuiControlBar
. We recommend usingEuiBottomBar
instead (#7122)EuiColorStops
. We recommend copying the component to your application if necessary (#7122)EuiNotificationEvent
. We recommend copying the component to your application if necessary (#7122)Breaking changes
EuiDescriptionList
'sgutterSize
prop torowGutterSize
(#7062)EuiDescriptionList
'srowGutterSize
prop now defaults to a size ofs
(was previouslym
) (#7062)Accessibility
EuiDescriptionListTitle
s to meet WCAG color contrast requirements (#7062)CSS-in-JS conversions
EuiKeyPadMenuItem
to Emotion; Removed$euiKeyPadMenuSize
and$euiKeyPadMenuMarginSize
(#7118)