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

[EuiDescriptionList] Update display to grid for vertical column option with new props #7062

Merged
merged 56 commits into from
Aug 28, 2023

Conversation

kyrspl
Copy link
Contributor

@kyrspl kyrspl commented Aug 9, 2023

Summary

This PR fixes issue #7011 and #6856. In addition, the following design decisions are included:

  • We add a columnGutterSize prop that controls vertical spacing between the 2 columns.
  • We rename gutterSize to rowGutterSize
  • We update the title's colors in dark mode for inline lists.
  • The display is changed to grid for vertical lists. This allows us to easily control both the vertical spacing between columns but also between rows

To do

  • Update display to grid for vertical lists
  • Add columnGutterSize prop
  • Control row-gap using rowGutterSize
  • Go through checklist below

Design

The idea here is to change the display to grid in order to have more flexible control over the columns and individual cells.
We introduce the columnGap prop to control vertical spacing between the columns.

Furthermore, it would be nice to control row spacing as well. We currently use gutterSize as a prop as well use the boolean compressed. Happy to discuss how we can add some logic around these two.

Finally, we're limiting the title's max-width to 380px (note: tbc) for optimal reading/scanning. Removed from PR

General checklist

  • Checked in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • Props have proper autodocs (using @default if default values are missing) and playground toggles
  • Added documentation
    - [ ] Checked Code Sandbox works for any docs examples
  • Added or updated jest and cypress tests
  • Checked for breaking changes and labeled appropriately
    - [ ] Checked for accessibility including keyboard-only and screenreader modes
  • Updated the Figma library counterpart
  • A changelog entry exists and is marked appropriately

@cla-checker-service
Copy link

cla-checker-service bot commented Aug 9, 2023

💚 CLA has been signed

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_7062_buildkite/

@kyrspl kyrspl marked this pull request as draft August 9, 2023 18:23
@kyrspl kyrspl changed the title WIP [EuiDescriptionList] Update display to grid for vertical column option with new props DRAFT [EuiDescriptionList] Update display to grid for vertical column option with new props Aug 9, 2023
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_7062_buildkite/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_7062_buildkite/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_7062_buildkite/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_7062_buildkite/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_7062_buildkite/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_7062_buildkite/

@kyrspl kyrspl marked this pull request as ready for review August 11, 2023 16:19
@kyrspl kyrspl changed the title DRAFT [EuiDescriptionList] Update display to grid for vertical column option with new props [EuiDescriptionList] Update display to grid for vertical column option with new props Aug 11, 2023
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_7062_buildkite/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_7062/

@kyrspl
Copy link
Contributor Author

kyrspl commented Aug 24, 2023

@cee-chen / @ryankeairns - Issue #7119 should take over the max-width topic.

@cee-chen
Copy link
Member

Thanks @kyrspl! Can you ping me either on Slack or in this GitHub thread once the max-width CSS has been removed from this PR? I'll re-review at that point.

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_7062/

Copy link
Member

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

🎉 Everything looks great and the code feels super clean to me! Thanks for taking the extra time to have deeper discussions on this Kyriakos, and for tackling #6856 as well!

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_7062/

@kyrspl
Copy link
Contributor Author

kyrspl commented Aug 25, 2023

Thanks for your help and guidance @cee-chen!

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_7062/

@kyrspl
Copy link
Contributor Author

kyrspl commented Aug 25, 2023

@ryankeairns Up to you for the final review :)

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_7062/

@kyrspl kyrspl enabled auto-merge (squash) August 28, 2023 16:39
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_7062/

@kyrspl kyrspl merged commit 6f03bd9 into elastic:main Aug 28, 2023
1 check passed
@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

@kyrspl kyrspl deleted the descriptionList_update_vertical branch August 29, 2023 10:27
jbudz pushed a commit to elastic/kibana that referenced this pull request Sep 5, 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` ([#7133](elastic/eui#7133))
- Updated `EuiDescriptionList` with new `columnWidths` prop
([#7146](elastic/eui#7146))

**Bug fixes**

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

**CSS-in-JS conversions**

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

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

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

**Deprecations**

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

**Breaking changes**

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

**Accessibility**

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

**CSS-in-JS conversions**

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

---------

Co-authored-by: Cee Chen <[email protected]>
Co-authored-by: Cee Chen <[email protected]>
Co-authored-by: kibanamachine <[email protected]>
Co-authored-by: Nikita Indik <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants