[Emotion] Remove .euiAccordionForm styles
#7154
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- isn't being set or documented anywhere in EUI itself, and Kibana usage is limited
- only 1 usage total in Kibana, we should specify that consumers should set this CSS themselves if they want it
…ngSize` prop + make opinionated hover text decoration custom CSS
+ update snapshots to include ID rerenders. Not totally sure why they changed, that's fun
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_7154/ |
1 similar comment
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_7154/ |
cee-chen
force-pushed
the
emotion/accordion-form
branch
from
96ed23f to
af32966
Compare
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_7154/ |
💚 Build Succeeded
History
|
breehall
approved these changes
Sep 5, 2023
There was a problem hiding this comment.
This is a great refactor and I'm glad we're removing the auxiliary classes. The new borders and buttonProps are more intuitive and I believe they will be enough for most cases consumers have. The props and their styling are straight forward and easy to understand.
I just have one question the comments - it doesn't impact approval here. Just curious about a bit of syntax we're using.
Comment on lines
+131
to
133
| borders: 'none' as const, | ||
| paddingSize: 'none' as const, | ||
| arrowDisplay: 'left' as const, |
There was a problem hiding this comment.
[Not feedback related] Do you mind explaining why we need as const when setting these props? I don't think I've seen it done like this before?
There was a problem hiding this comment.
If we don't, Typescript starts throwing errors:
This is due to defaultProps being an object. Typescript can detect top-level strings as matching enums, but diving into nested object keys to detect enums is (I believe) too costly for them performance-wise to do by default, so we essentially need the as const here to tell Typescript "hey this isn't just a random string value, it's an enum/const value".
Ikuni17
pushed a commit
to elastic/kibana
that referenced
this pull request
Sep 12, 2023
Major changes in this PR:
## Removal of `.euiAccordionForm` classNames
EUI is moving away from providing global `classNames` styles for
components - where possible, we want to provide props as opposed to
styles. As part of our ongoing Emotion conversion, we have removed the
following `EuiAccordion`-specific classes:
- `.euiAccordionForm` (replaced with `borders="horizontal"`)
- `.euiAccordionForm__button` (replaced with `buttonProps={{
paddingSize: 'm' }}`)
- `.euiAccordionForm__title` styles - this was only removing text
underlines on hover. If still desired, re-add this behavior with custom
CSS.
- `.euiAccordionForm__extraAction` - there was 1 usage of this in Kibana
in Watcher, which was converted to one-off custom inline Emotion CSS
instead.
This change accounts for the first 3-4 commits in the PR. ⚠️ If your
team was one of the 4-5 teams affected by this change, we would greatly
appreciate your help QAing the UI and ensuring it looks as desired/the
same as before.
## Fixed `EuiHeader` affordance
The Sass `euiHeaderAffordForFixed` mixin has been deprecated and
replaced by a global `--euiFixedHeadersOffset` CSS variable. This
variable updates independently and dynamically based on the number of
fixed headers on the page, and is usable by both Sass and Emotion. All
underlying EUI components that need to account for fixed headers (such
as flyouts and page sidebars/templates) have been updated to consume
this new variable.
This change cleans up a great deal of Sass code, and is incidentally
extremely timely with serverless efforts (as serverless has only one
fixed header and the classic Kibana chrome has two).
This change constitutes a majority of the commits in this PR, which
involve removing various fixed Sass header variables and replacing them
with the new CSS variable. I strongly recommend [reviewing changes by
commit if
possible](https://github.com/elastic/kibana/pull/165790/commits) to see
any associated changes that make sense together with any of your touched
file(s). ⚠️ If your team was affected by this change (primarily due to
custom header layouts), your help would be greatly appreciated QAing
your app to ensure no UI regressions in that regard have occurred.
---
`v88.1.0`⏩ `v88.2.0`
## [`88.2.0`](https://github.com/elastic/eui/tree/v88.2.0)
- Added a new `EuiTextTruncate` component, which provides custom
truncation options beyond native CSS
([#7116](elastic/eui#7116))
- Fixed-positioned `EuiHeader`s now set a global CSS
`--euiFixedHeadersOffset` variable, which updates dynamically based on
the number of fixed headers on the page.
([#7144](elastic/eui#7144))
- `EuiFlyout`s now dynamically set their position, height, and mask
based on the number of fixed headers on the page.
([#7144](elastic/eui#7144))
- Sticky-positioned `EuiPageSidebar`s now dynamically set their position
and height based on the number of fixed headers on the page. This can
still be overridden via the `sticky.offset` prop if needed.
([#7144](elastic/eui#7144))
- `EuiPageTemplate` now dynamically offsets content from any fixed
headers on the page. This can still be overridden via the `offset` prop
if needed. ([#7144](elastic/eui#7144))
- Updated `EuiAccordion` with a new `borders` prop
([#7154](elastic/eui#7154))
- Updated `EuiAccordion` with a new `buttonProps.paddingSize` prop
([#7154](elastic/eui#7154))
**Deprecations**
- Deprecated the Sass `euiHeaderAffordForFixed` mixin. Use the new
global CSS `var(--euiFixedHeadersOffset)` variable instead.
([#7144](elastic/eui#7144))
**CSS-in-JS conversions**
- Except for generic CSS utilities, EUI is moving away from providing
global `classNames` that are component-specific. As part of this effort,
we have removed the following `EuiAccordion`-specific classes:
([#7154](elastic/eui#7154))
- Removed `.euiAccordionForm` styles. Use the `borders="horizontal"`
prop instead
- Removed `.euiAccordionForm__button` styles. Use the `buttonProps={{
paddingSize: 'm' }}` prop instead
- Removed `.euiAccordionForm__extraAction` styles. Convert this to your
own custom CSS if necessary.
- Removed `.euiAccordionForm__title` styles. Convert this to your own
custom CSS if necessary.
---------
Co-authored-by: Stratoula Kalafateli <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
closes #6386
This was a tricky PR to "convert" because none of these Sass-generated classes are actually used in EUI itself; they're classes we generated and told consumers to use directly on EuiAccordion (an architecture we're trying to move away from with Emotion).
Thankfully, there's just 4 or so internal Elastic usages of these classes - 3 in Kibana and 1 in support portal. I based my decisions on what to turn into a generic prop on those usages. Please follow along by commit if possible.
QA
General checklist
[ ] Updated the Figma library counterpart- thankfully it looks like this pattern isn't in our Figma librarySkipping the Emotion checklist for this since it's almost entirely all Kibana downstream updates