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.

Sign up for GitHub

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

Jump to bottom

[EuiFilterGroup] Fix responsive styles #6983

Merged
merged 3 commits into from
Jul 24, 2023
Merged

[EuiFilterGroup] Fix responsive styles #6983

merged 3 commits into from
Jul 24, 2023

Conversation

cee-chen
Copy link
Contributor

@cee-chen cee-chen commented Jul 24, 2023 •
edited
Loading

Summary

🤦 I messed up the syntax for this during #6982, and now EuiFilterGroup is full width at all breakpoints when it should only be full-width at the xs->s breakpoint.

Above m breakpoint (width):

Broken on prod:

After fix:

Below m breakpoint (flex wrapping):

Broken on prod:

After fix:

QA

General checklist

  • Checked in mobile
  • A changelog entry exists and is marked appropriately

cee-chen added 2 commits July 24, 2023 08:03
@cee-chen
Fix EuiFilterGroup's responsive styling
6b842c6 
- I messed up the breakpoint interpretation/logic during its Emotion conversion
@cee-chen
changelog
0ee5a07
@cee-chen cee-chen marked this pull request as ready for review July 24, 2023 15:13
@cee-chen cee-chen requested a review from tkajtoch July 24, 2023 15:13
cee-chen
cee-chen commented Jul 24, 2023
View reviewed changes
src/components/filter_group/filter_group.styles.ts
@@ -51,10 +47,10 @@ export const euiFilterGroupStyles = (euiThemeContext: UseEuiTheme) => {
}
}

${euiMaxBreakpoint(euiThemeContext, 's')} {
${euiBreakpoint(euiThemeContext, ['xs', 's'])} {
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 always forget how dang unintuitively our breakpoint mixin is written, at least for me - I personally see a "breakpoint" as a single 1D point, the previous EUI team apparently saw a breakpoint as a 2D "from X to Y size screen". So this breakpoint actually translates to a max-width of the m breakpoint, not the s breakpoint.

This leads to me occasionally fucking up converting Sass media queries - hopefully I'll get back into the muscle memory of it after this PR 🤦

@cee-chen
Copy link
Contributor Author

@tkajtoch We'll need to backport this into your current Kibana upgrade (it may or may not be causing failures in your PR; either way I would feel bad if Kibana devs saw this or had to deal with this).

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

1Copenut
1Copenut approved these changes Jul 24, 2023
View reviewed changes
Copy link
Contributor

@1Copenut 1Copenut left a comment

Choose a reason for hiding this comment

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

👍 LGTM! I tested briefly in Chrome, Edge, and Firefox. Toggled through the mobile emulator and full-width to ensure breakpoint changes.

@cee-chen cee-chen merged commit 2121e09 into elastic:main Jul 24, 2023
@cee-chen cee-chen deleted the emotion/filter-group-fix branch July 24, 2023 18:07
@cee-chen cee-chen mentioned this pull request Jul 24, 2023
Merged
Ikuni17 pushed a commit to elastic/kibana that referenced this pull request Jul 27, 2023
@tkajtoch @cee-chen @kibanamachine
Upgrade @elastic/eui to 85.0.1 (#162209)
1c42ee9 
## Summary

`eui@84.0.0` ⏩ `eui@85.0.1`

## [`85.0.1`](https://github.com/elastic/eui/tree/v85.0.1)

**Bug fixes**

- Fixed `EuiFilterGroup`'s responsive styles
([#6983](elastic/eui#6983))

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

- Updated `EuiThemeProvider` to set an Emotion theme context that
returns the values of `useEuiTheme()`
([#6913](elastic/eui#6913))
- Added `size` prop to `EuiStepsHorizontal`, defaulting to the previous
size of `m` ([#6928](elastic/eui#6928))
- Added new `s` sizing to `EuiStepsHorizontal`
([#6928](elastic/eui#6928))
- Added `at` and `key` icon glyphs.
([#6934](elastic/eui#6934))
- Added a new `cloneElementWithCss` Emotion utility
([#6939](elastic/eui#6939))
- Updated `EuiPopover` to allow consumer control of all `focusTrapProps`
([#6955](elastic/eui#6955))

**Bug fixes**

- Fixed `EuiDataGrid` height calculation bug when browser zoom levels
are not 100% ([#6895](elastic/eui#6895))
- Fixed `EuiTab` not correctly passing selection color state to
`prepend` and `append` children
([#6938](elastic/eui#6938))
- Fixed `EuiInputPopover` to allow consumer control of its focus trap
via `focusTrapProps` ([#6955](elastic/eui#6955))

**Breaking changes**

- `EuiProvider` will no longer render multiple or duplicate nested
instances of itself. If a nested `EuiProvider` is detected, that
instance will return early without further processing, and will warn if
configured to do so via `setEuiDevProviderWarning`. For nested theming,
use `EuiThemeProvider` instead.
([#6949](elastic/eui#6949))
- Removed `onTrapDeactivation` prop from `EuiPopover`. Use
`focusTrapProps.onDeactivation` instead
([#6955](elastic/eui#6955))

**CSS-in-JS conversions**

- Converted `EuiFilterGroup` and `EuiFilterButton` to Emotion; Removed
styles attached to `.euiFilterGroup__popoverPanel`
([#6957](elastic/eui#6957))

---------

Co-authored-by: Cee Chen <constance.chen@elastic.co>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@breehall breehall mentioned this pull request Jul 27, 2023
Merged
ThomThomson pushed a commit to ThomThomson/kibana that referenced this pull request Aug 1, 2023
@tkajtoch @cee-chen @kibanamachine
Upgrade @elastic/eui to 85.0.1 (elastic#162209)
5f2e2ba 
## Summary

`eui@84.0.0` ⏩ `eui@85.0.1`

## [`85.0.1`](https://github.com/elastic/eui/tree/v85.0.1)

**Bug fixes**

- Fixed `EuiFilterGroup`'s responsive styles
([elastic#6983](elastic/eui#6983))

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

- Updated `EuiThemeProvider` to set an Emotion theme context that
returns the values of `useEuiTheme()`
([elastic#6913](elastic/eui#6913))
- Added `size` prop to `EuiStepsHorizontal`, defaulting to the previous
size of `m` ([elastic#6928](elastic/eui#6928))
- Added new `s` sizing to `EuiStepsHorizontal`
([elastic#6928](elastic/eui#6928))
- Added `at` and `key` icon glyphs.
([elastic#6934](elastic/eui#6934))
- Added a new `cloneElementWithCss` Emotion utility
([elastic#6939](elastic/eui#6939))
- Updated `EuiPopover` to allow consumer control of all `focusTrapProps`
([elastic#6955](elastic/eui#6955))

**Bug fixes**

- Fixed `EuiDataGrid` height calculation bug when browser zoom levels
are not 100% ([elastic#6895](elastic/eui#6895))
- Fixed `EuiTab` not correctly passing selection color state to
`prepend` and `append` children
([elastic#6938](elastic/eui#6938))
- Fixed `EuiInputPopover` to allow consumer control of its focus trap
via `focusTrapProps` ([elastic#6955](elastic/eui#6955))

**Breaking changes**

- `EuiProvider` will no longer render multiple or duplicate nested
instances of itself. If a nested `EuiProvider` is detected, that
instance will return early without further processing, and will warn if
configured to do so via `setEuiDevProviderWarning`. For nested theming,
use `EuiThemeProvider` instead.
([elastic#6949](elastic/eui#6949))
- Removed `onTrapDeactivation` prop from `EuiPopover`. Use
`focusTrapProps.onDeactivation` instead
([elastic#6955](elastic/eui#6955))

**CSS-in-JS conversions**

- Converted `EuiFilterGroup` and `EuiFilterButton` to Emotion; Removed
styles attached to `.euiFilterGroup__popoverPanel`
([elastic#6957](elastic/eui#6957))

---------

Co-authored-by: Cee Chen <constance.chen@elastic.co>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
mistic pushed a commit to elastic/kibana that referenced this pull request Aug 3, 2023
@breehall @cee-chen @kibanamachine
Upgrade EUI to v85.1.0 (#162660)
68fec4b 
`85.0.0` ➡️ `85.1.0`

⚠️ The biggest change in this PR by far is the **removal** of several
`EuiFilterSelectItem` CSS classes and styles associated with those
classes. EUI has previously exported component-specific CSS classes for
usage such as `.euiFilterSelect__items`,
`.euiFilterGroup__popoverPanel`, or `.euiAccordionForm`.

❌ **As of our move to CSS-in-JS, we are actively moving away from this
architecture**. The goal is to either provide either a component or prop
directly to you instead of a CSS class. As a reminder, our classNames
are not guaranteed APIs and are subject to change without warning -
should you need to tweak or customize EUI's styling, we strongly
recommend passing in your own `className`.

💬 Moving forward, EUI will attempted to convert any usages of removed
CSS classes and their direct usages in Kibana for you. In most cases,
we'll hopefully be able to take the correct path of using a component or
prop instead. In cases where we cannot or significant/complex changes
are required, we may temporarily convert the CSS to a static CSS-in-JS
usage instead and add a TODO asking the relevant team to address this in
their own time (for example, the deprecation of `EuiFilterSelectItem`
and its conversion to `EuiSelectable`).

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

- Updated `EuiComboBox`'s `options` to accept `option.append` and
`option.prepend` props
([#6953](elastic/eui#6953))
- Updated deprecated `.substr()` usages to `.substring()`
([#6954](elastic/eui#6954))
- Updated `EuiInlineEdit`'s read mode button to include a title tooltip,
increasing readability of truncated text
([#6966](elastic/eui#6966))

**Bug fixes**

- Fixed `EuiFilterGroup`'s responsive styles
([#6983](elastic/eui#6983))

**Deprecations**

- Deprecated `EuiFilterSelectItem`; Use `EuiSelectable` instead
([#6982](elastic/eui#6982))

**CSS-in-JS conversions**

- Converted `EuiFilterSelectItem` to Emotion
([#6982](elastic/eui#6982))
- Removed `.euiFilterSelect__items` CSS; Use `EuiSelectable` instead
([#6982](elastic/eui#6982))
- Removed `.euiFilterSelect__note` and `.euiFilterSelect__noteContent`
CSS; Use `EuiSelectableMessage` instead
([#6982](elastic/eui#6982))
- Added `focus.transparency` and `focus.backgroundColor` theme tokens
([#6984](elastic/eui#6984))

---------

Co-authored-by: Cee Chen <constance.chen@elastic.co>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@1Copenut 1Copenut 1Copenut approved these changes

@tkajtoch tkajtoch Awaiting requested review from tkajtoch

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cee-chen @kibanamachine @1Copenut