`EuiAccordion` convert Sass to Emotion CSS by 1Copenut · Pull Request #5826 · elastic/eui

1Copenut · 2022-04-20T21:16:28Z

Summary

PR to move EuiAccordion to Emotion CSS. ~~This is dependent on @cchaos work on font-size and line-height. I'll add those items as they're ready.~~

I'm opting to remove the breaking change label. We've decided to hold off refactoring the accordion in form work to another PR:

I could use a hand from reviewers to determine if this is still a breaking change. My hunch is no, because the work for EuiAccordion as part of a form has been walked back and will be a separate PR.

#5826 (comment)

Checklist

* Fixed the WithEuiTheme HOC export TypeScript errors. * Refactoring Emotion to exports for relevant tags. * Adding named export for child wrapper. * Moving last stateful CSS to Emotion before adding padding prop. * Adding last Emotion styles using paddingSize prop. * Small refactoring of types before updating tests. WIP. Need to add TS props for passing builds.

kibanamachine · 2022-04-20T21:59:59Z

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

…-accordion-emotion

kibanamachine · 2022-04-21T19:18:33Z

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

kibanamachine · 2022-04-21T19:30:55Z

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

…-accordion-emotion

kibanamachine · 2022-04-26T17:48:24Z

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

1Copenut · 2022-04-26T18:47:06Z

I could use a hand from reviewers to determine if this is still a breaking change. My hunch is no, because the work for EuiAccordion as part of a form has been walked back and will be a separate PR.

kibanamachine · 2022-04-26T19:14:56Z

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

cchaos

Something has gone wrong with the Playground 😬

upcoming_changelogs/5826.md

src/components/accordion/accordion.tsx

src/components/accordion/accordion.styles.ts

kibanamachine · 2022-04-26T21:58:15Z

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

thompsongl · 2022-04-27T16:15:31Z

Something has gone wrong with the Playground

@1Copenut I'll look into this

1Copenut · 2022-04-27T16:54:41Z

Something has gone wrong with the Playground

@1Copenut I'll look into this

@thompsongl There were two errors initially. I figured out the ID issue by adding a prop type to the accordion playground.js file:

propsToUse.id = {
  ...propsToUse.id,
  value: htmlIdGenerator('generated')(),
+ type: PropTypes.String,
};

The other I have no goodly idea.

kibanamachine · 2022-04-27T20:52:40Z

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

kibanamachine · 2022-04-28T18:09:01Z

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

cchaos

Playground

One oddness I'm trying to understand is that while in the Playground, when toggling isLoading to true, the spinner never shows up (even though it works fine in the docs). This isn't unique to this PR as it occurs in production. However, the difference I am seeing between prod and this PR is that when isLoading=true and arrowDisplay="none" I'm still seeing the arrow:

This PR

Prod

Snapshot

I feel strongly that we should fix this before we merge this PR, but the snapshots are rendering the entire JSON output of the theme.

Snippet

exports[`EuiAccordion behavior closes when clicked twice 1`] = `
<WithEuiTheme(EuiAccordionClass)
  id="18"
>
  <EuiAccordionClass
    arrowDisplay="left"
    buttonElement="button"
    element="div"
    id="18"
    initialIsOpen={false}
    isLoading={false}
    isLoadingMessage={false}
    paddingSize="none"
    theme={
      Object {
        "colorMode": "LIGHT",
        "euiTheme": Object {
          "animation": Object {
            "bounce": "cubic-bezier(.34, 1.61, .7, 1)",
            "extraFast": "90ms",
            "extraSlow": "500ms",
            "fast": "150ms",
            "normal": "250ms",
            "resistance": "cubic-bezier(.694, .0482, .335, 1)",
            "slow": "350ms",
          },
          "base": 16,
          "border": Object {
            "color": "#D3DAE6",
            "editable": "2px dotted #D3DAE6",
            "radius": Object {
              "medium": "6px",
              "small": "4px",
            },
            "thick": "2px solid #D3DAE6",
            "thin": "1px solid #D3DAE6",
            "width": Object {
              "thick": "2px",
              "thin": "1px",
            },
          },

Styles based on props

Lastly, we should really only be doing the prop checking in the component TSX. The styles should purely be key: css pairs where key is the value of the prop (like the paddingSize one). See this section of the wiki: https://github.com/elastic/eui/blob/main/wiki/writing-styles-with-emotion.md#component-props-that-enable-styles

src/components/accordion/accordion.styles.ts

src/components/accordion/accordion.tsx

src/components/accordion/accordion.styles.ts

src/components/accordion/accordion.tsx

kibanamachine · 2022-04-28T21:37:20Z

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

…-accordion-emotion

kibanamachine · 2022-05-03T18:34:00Z

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

kibanamachine · 2022-05-09T21:28:58Z

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

1Copenut · 2022-05-09T21:30:43Z

@cchaos I confirmed the loading spinner is working on localhost playground if I toggle isLoading and add a a string like "Loading" to the isLoadingMessage prop box.

cchaos · 2022-05-10T17:29:31Z

@1Copenut I've located the isLoading problem to the fact that it does nothing unless extraAction or isLoadingMessage is defined. It seems this logic is in place in production right now so it's not related to this PR specifically. However, it may be a nice chance to quickly change the logic so that no matter the state of extraAction if isLoading=true you'll always see the loading spinner. Right now simply adding isLoading=true to a basic instance does nothing.

1Copenut · 2022-05-10T22:35:22Z

@cchaos Logged the request to simplify loading prop logic in #5893. Looking at the code, it was going to be slightly more involved than I first thought. Capturing your comment so I can pick it up again tomorrow.

cchaos

Design code LGTM (one tiny suggestion). I double checked any effects on EuiCollapsibleNavGroup and EuiNotificationEvent as well.

Worth getting an eng to look over the class stuff.

cchaos · 2022-05-16T15:13:29Z

src/components/accordion/accordion.styles.ts

+  euiAccordion__spinner: css``,
+  isLoading: css`
+    margin-right: ${euiTheme.size.xs};
+  `,


Nit: the spinner only shows up when isLoading is true, so there's no need for this qualifier, it can just be applied directly to the base selector.

Suggested change

euiAccordion__spinner: css``,

isLoading: css`

margin-right: ${euiTheme.size.xs};

`,

euiAccordion__spinner: css`

margin-right: ${euiTheme.size.xs};

`,

Thanks Caroline. Change made and snapshot updated.

kibanamachine · 2022-05-16T16:15:38Z

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

cchaos · 2022-05-16T16:57:09Z

I'm pushing this PR off to Stack 8.4 release because I know that changes to EuiAccordion in the past have caused consumers grief. So I worry about this one so close to 8.3 FF.

thompsongl

Code changes LGTM

I'm pushing this PR off to Stack 8.4 release because I know that changes to EuiAccordion in the past have caused consumers grief. So I worry about this one so close to 8.3 FF.

Probably safe to merge this. v57 is the last one going into 8.3, and any backports won't include the changes here. But feel free to wait til next week if you want.

breehall

This looks good to me! Great work on this Trevor!

1Copenut added 2 commits April 20, 2022 15:07

Updating TS errors and unit tests for EuiAccordion.

1c94a47

1Copenut added the emotion label Apr 20, 2022

1Copenut requested review from breehall, cchaos and thompsongl April 20, 2022 21:16

1Copenut self-assigned this Apr 20, 2022

Fixing upstream/main conflict in CSS file.

00a5be0

1Copenut added 3 commits April 21, 2022 12:11

Merge remote-tracking branch 'upstream/main' into feature/tpierce-eui…

da13a2e

…-accordion-emotion

Adding final Emotion hooks to style EuiAccordion.

f77d943

Adding a changelog entry manually.

a19c7cd

1Copenut added the breaking change PRs with breaking changes. (Don't delete - used for automation) label Apr 21, 2022

thompsongl mentioned this pull request Apr 25, 2022

[Meta][CSS-in-JS] Component conversions #5685

Closed

49 tasks

1Copenut added 2 commits April 26, 2022 12:14

Removing EuiAccordion styles from Sass.

0225666

Merge remote-tracking branch 'upstream/main' into feature/tpierce-eui…

9858bea

…-accordion-emotion

Refactoring line-height to use EuiTheme.

407ca6c

1Copenut marked this pull request as ready for review April 26, 2022 18:44

1Copenut changed the title ~~[WIP] EuiAccordion convert Sass to Emotion CSS~~ EuiAccordion convert Sass to Emotion CSS Apr 26, 2022

cchaos suggested changes Apr 26, 2022

View reviewed changes

Refactor 1 from PR feedback.

ff4d9bb

use unwrapped class; account for theme prop

1e1fd31

Updating snapshots after pulling upstreamin main branch.

c9bdbee

1Copenut requested a review from cchaos April 27, 2022 20:23

Merging upstream/main to fix a snapshot conflict.

0120cb7

cchaos suggested changes Apr 28, 2022

View reviewed changes

Removing props from Emotion conditional CSS rendering.

8c10728

1Copenut added 2 commits May 3, 2022 13:01

Updating snapshots to not render Emotion sourcemaps.

cd0d21e

Merge remote-tracking branch 'upstream/main' into feature/tpierce-eui…

ce41aa2

…-accordion-emotion

Merge branch 'main' into feature/tpierce-eui-accordion-emotion

ae7910e

1Copenut requested a review from cchaos May 9, 2022 21:30

1Copenut removed the breaking change PRs with breaking changes. (Don't delete - used for automation) label May 10, 2022

1Copenut mentioned this pull request May 10, 2022

[EuiAccordion] Refactor to show the spinner as soon as isLoading prop is passed #5893

Closed

1Copenut mentioned this pull request May 11, 2022

Simplying EuiAccordion isLoading and isLoadingMessage logic #5896

Merged

11 tasks

cchaos approved these changes May 16, 2022

View reviewed changes

Simplified isLoading CSS-in-JS declaration.

e5538ee

cchaos added this to the Elastic Stack 8.4 milestone May 16, 2022

thompsongl approved these changes May 16, 2022

View reviewed changes

breehall approved these changes May 17, 2022

View reviewed changes

1Copenut merged commit 922a789 into elastic:main May 17, 2022

1Copenut deleted the feature/tpierce-eui-accordion-emotion branch May 17, 2022 19:32

thompsongl mentioned this pull request Jun 8, 2022

Upgrade EUI to v59.0.1 elastic/kibana#133927

Merged

Conversation

1Copenut commented Apr 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

kibanamachine commented Apr 20, 2022

Uh oh!

kibanamachine commented Apr 21, 2022

Uh oh!

kibanamachine commented Apr 21, 2022

Uh oh!

kibanamachine commented Apr 26, 2022

Uh oh!

1Copenut commented Apr 26, 2022

Uh oh!

kibanamachine commented Apr 26, 2022

Uh oh!

cchaos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kibanamachine commented Apr 26, 2022

Uh oh!

thompsongl commented Apr 27, 2022

Uh oh!

1Copenut commented Apr 27, 2022

Uh oh!

kibanamachine commented Apr 27, 2022

Uh oh!

kibanamachine commented Apr 28, 2022

Uh oh!

cchaos left a comment

Choose a reason for hiding this comment

Playground

Snapshot

Styles based on props

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kibanamachine commented Apr 28, 2022

Uh oh!

kibanamachine commented May 3, 2022

Uh oh!

kibanamachine commented May 9, 2022

Uh oh!

1Copenut commented May 9, 2022

Uh oh!

cchaos commented May 10, 2022

Uh oh!

1Copenut commented May 10, 2022

Uh oh!

cchaos left a comment

Choose a reason for hiding this comment

Uh oh!

cchaos May 16, 2022

Choose a reason for hiding this comment

Uh oh!

1Copenut May 16, 2022

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented May 16, 2022

Uh oh!

cchaos commented May 16, 2022

Uh oh!

thompsongl left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1Copenut commented Apr 20, 2022 •

edited

Loading

thompsongl left a comment •

edited

Loading