[Emotion] Converted EuiLoadingChart #5821

cchaos · 2022-04-19T19:23:47Z

Summary

Converted margins to use either gap or logical property
Tried adding renderWithStyles() in test (commented out for now)
Uses Emotion keyframes
Cleaned up classes and using for loop for bars

Okay descrepencies (See screenshot below)

By using a function based on index, the math was simpler to adjust the non-animated version to trend downward instead of upward.
By switching to the gap property, the first bar no longer as an added margin left so all the loaders a slightly skinnier and sit better in their full bounds.

Helper adjustments / additions

Added tintOrShade and shadeOrTint functions
Updated canAnimate functions to accept and return the css object (@thompsongl to look into this further)

Checklist

Checked in both light and dark modes
[ ] Checked in mobile
Checked in Chrome, Safari, Edge, and Firefox
[ ] Props have proper autodocs 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

- Added tintOrShade and shadeOrTint functions - Converted margins to use either `gap` or logical property - Couldn’t convert `keyframes` yet

…ding_chart

- Updated `canAnimate` functions to accept and return the css object

cchaos · 2022-04-19T19:46:41Z

Things to look out for when moving styles

Anything in the backlog that could be a quick fix for that component? [EuiLoading] Accessibility props #4814
Convert global Sass vars to JS version like sizing
~~[ ] Convert component-specific Sass vars to exported JS versions~~
Convert side specific padding, margin, and position to -inline and -block (Logical properties)
Reduce specificity where possible (usually by reducing class name chaining)
Use gap property to add margin between items if using flex
~~[ ] Can any still existing .js files be converted to TS?~~

QA

Do className, css, and style props work properly when passed by the consumer
Does the rendered class read as expected

cchaos · 2022-04-19T19:48:47Z

@1Copenut I've updated a few props on this specific loading component according to this ticket: #4814

Added role="progressbar"
Added default aria-label="Loading"

I didn't do the full documentation or any of the children bit since that's more involved. But wanted to get you approval on this iterative chage.

kibanamachine · 2022-04-19T19:58:06Z

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

…ding_chart

kibanamachine · 2022-04-19T20:41:21Z

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

kibanamachine · 2022-04-19T21:55:54Z

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

elizabetdev

LGTM! 🎉

Tested in Chrome, Safari, and Firefox. Also tested with reduce motion turned on.

src/services/color/manipulation.ts

src/components/loading/loading_chart.test.tsx

…ding_chart

kibanamachine · 2022-04-20T21:22:32Z

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

1Copenut

LGTM! I added one review comment as a thought that I'll develop into a full ticket shortly.

1Copenut · 2022-04-25T15:05:55Z

src/components/loading/loading_chart.tsx

+    <span
+      className={classes}
+      css={cssStyles}
+      role="progressbar"


I agree with this iterative approach. Adding the role="progressbar" and aria-label="Loading" give clues that content is being fetched async. This technique assumes screen reader users are navigating node to node, using the virtual/browse mode. Users navigating by focus with the TAB key will not land on the loading indicator. Users who are listening for cues via live regions will also never hear this information.

I'm going to open a spike ticket to explore this concept in a future breaking change. I'd like to see us use the aria-busy attribute and move to a role="status" for these loading indicators. That way screen readers will announce changes as a live region without requiring us to manage focus or users to seek them out.

🙌 Thank you! I'm converting the rest of the loading components too. Should I add these same properties?

Yes, let's add the role="progressbar" and aria-label="Loading" consistently. Great call!

…ding_chart # Conflicts: # src/global_styling/variables/_animations.ts

kibanamachine · 2022-04-25T17:04:06Z

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

…ding_chart

kibanamachine · 2022-04-25T19:39:34Z

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

cchaos added 5 commits April 19, 2022 13:42

[EuiLoadingChart] Initial Emotion conversion

e8fe9f6

- Added tintOrShade and shadeOrTint functions - Converted margins to use either `gap` or logical property - Couldn’t convert `keyframes` yet

Tried adding renderWithStyles()

97fa2f0

Merge remote-tracking branch 'origin/main' into css-in-js-convert/loa…

1a694f1

…ding_chart

Using Emotion keyframes

d79c035

- Updated `canAnimate` functions to accept and return the css object

Cleanup classes and for loop for bars

5fbe13e

cchaos changed the title ~~[Emption] Converted EuiLoadingChart~~ [Emotion] Converted EuiLoadingChart Apr 19, 2022

cchaos added 3 commits April 19, 2022 15:27

cl

76046f8

Fix some docs

5a95cb4

Improved accessibility

f4e367b

Merge remote-tracking branch 'origin/main' into css-in-js-convert/loa…

bfdb3d9

…ding_chart

unrelated snap?

2404a41

cchaos mentioned this pull request Apr 19, 2022

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

Closed

49 tasks

elizabetdev approved these changes Apr 20, 2022

View reviewed changes

src/services/color/manipulation.ts Outdated Show resolved Hide resolved

elizabetdev reviewed Apr 20, 2022

View reviewed changes

src/services/color/manipulation.ts Outdated Show resolved Hide resolved

elizabetdev reviewed Apr 20, 2022

View reviewed changes

src/components/loading/loading_chart.test.tsx Show resolved Hide resolved

cchaos added 2 commits April 20, 2022 16:49

Merge remote-tracking branch 'origin/main' into css-in-js-convert/loa…

5834d16

…ding_chart

Fix TS and test

3ada7f9

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

1Copenut approved these changes Apr 25, 2022

View reviewed changes

Merge remote-tracking branch 'origin/main' into css-in-js-convert/loa…

6985b96

…ding_chart # Conflicts: # src/global_styling/variables/_animations.ts

cchaos added 2 commits April 25, 2022 15:05

Merge remote-tracking branch 'origin/main' into css-in-js-convert/loa…

961fafb

…ding_chart

Fix import

0b15394

cchaos enabled auto-merge (squash) April 25, 2022 19:16

cchaos merged commit 042a260 into elastic:main Apr 25, 2022

cchaos deleted the css-in-js-convert/loading_chart branch April 25, 2022 20:15

This was referenced Apr 26, 2022

[EuiLoadingChart][SCREENREADER]: Consider updating the loading indicators' role to "status" and adding aria-busy attribute #5843

Open

Upgrade EUI to v55.1.2 elastic/kibana#131203

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Emotion] Converted EuiLoadingChart #5821

[Emotion] Converted EuiLoadingChart #5821

cchaos commented Apr 19, 2022 •

edited

Loading

cchaos commented Apr 19, 2022

cchaos commented Apr 19, 2022

kibanamachine commented Apr 19, 2022

kibanamachine commented Apr 19, 2022

kibanamachine commented Apr 19, 2022

elizabetdev left a comment

kibanamachine commented Apr 20, 2022

1Copenut left a comment

1Copenut Apr 25, 2022

cchaos Apr 25, 2022

1Copenut Apr 25, 2022

kibanamachine commented Apr 25, 2022

kibanamachine commented Apr 25, 2022

[Emotion] Converted EuiLoadingChart #5821

[Emotion] Converted EuiLoadingChart #5821

Conversation

cchaos commented Apr 19, 2022 • edited Loading

Summary

Okay descrepencies (See screenshot below)

Helper adjustments / additions

Checklist

cchaos commented Apr 19, 2022

Things to look out for when moving styles

QA

cchaos commented Apr 19, 2022

kibanamachine commented Apr 19, 2022

kibanamachine commented Apr 19, 2022

kibanamachine commented Apr 19, 2022

elizabetdev left a comment

Choose a reason for hiding this comment

kibanamachine commented Apr 20, 2022

1Copenut left a comment

Choose a reason for hiding this comment

1Copenut Apr 25, 2022

Choose a reason for hiding this comment

cchaos Apr 25, 2022

Choose a reason for hiding this comment

1Copenut Apr 25, 2022

Choose a reason for hiding this comment

kibanamachine commented Apr 25, 2022

kibanamachine commented Apr 25, 2022

cchaos commented Apr 19, 2022 •

edited

Loading