Add isLoading and isLoadingMessage props to EuiAccordion by andreadelrio · Pull Request #3879 · elastic/eui

andreadelrio · 2020-08-09T22:27:09Z

Summary

Setting isLoading to true, adds a EuiLoadingSpinner to the accordion's triggerWrapper. The content is also replaced with a EuiLoadingSpinner which can take a custom loading message via isLoadingMessage.

Fixes #3773

Checklist

Check against all themes for compatibility in both light and dark modes
Checked in mobile
Checked in Chrome, Safari, Edge, and Firefox
Props have proper autodocs
Added documentation
~~- [ ] Checked Code Sandbox works for the any docs examples~~
Added or updated jest tests
~~- [ ] Checked for breaking changes and labeled appropriately~~
~~- [ ] Checked for accessibility including keyboard-only and screenreader modes~~
A changelog entry exists and is marked appropriately

kibanamachine · 2020-08-09T22:53:19Z

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

elizabetdev

Hi @andreadelrio! Thanks for adding these props ! 🎉

I found a few issues. I think the loading could be vertical centered:

Also, should we have a default isLoadingMessage? Or we could end up having cases like the following one:

Also when I added the isLoading and isLoadingMessage to the first example the isLoadingMessage gets a little bit cut:

andreadelrio · 2020-08-12T01:01:30Z

@miukimiu thanks for the review! I added a default message, fixed styling and also fixed an issue where isLoading wasn't working unless there was an extraAction being passed.

kibanamachine · 2020-08-12T01:11:57Z

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

kibanamachine · 2020-08-12T01:55:05Z

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

kibanamachine · 2020-08-12T05:24:24Z

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

elizabetdev

Thanks for making changes. Tested again and LGTM! 🎉

I just noticed the build is failing when running the a11y-testing.

src-docs/src/views/accordion/accordion_isLoading.js

cchaos

I'd have one suggestion to ask for. In Lens, we use accordions and also currently fake the loading state similar to what you're doing with the extraAction piece. However, these loading moments don't necessarily mean that everything inside the accordion is going to change. In fact, we want to be sure that they can still interact with the original content while more content is being loaded.

My ask is if you can make having the loading message replace the content entirely as optional. The way I would do that is by allowing the isLoadingMessage prop to also accept a boolean, and I'd even go further to suggest defaulting to false (off) just because I'm not sure that it's going to be the most common need. I left some code comments on how you could approach it.

src/components/accordion/accordion.tsx

src/components/accordion/accordion.test.tsx

cchaos · 2020-08-12T14:34:38Z

src-docs/src/views/accordion/accordion_isLoading.js

+    buttonContent="Click to open"
+    extraAction={<EuiButton size="s">Extra action!</EuiButton>}
+    isLoading
+    isLoadingMessage="Loading, please wait">


By adding this custom message it looks to the user that it may be the default. I'd suggest keeping only the default in the example or add a toggle that will showcase what the default vs a custom message may look like.

andreadelrio · 2020-08-12T21:49:17Z

I'd have one suggestion to ask for. In Lens, we use accordions and also currently fake the loading state similar to what you're doing with the extraAction piece. However, these loading moments don't necessarily mean that everything inside the accordion is going to change. In fact, we want to be sure that they can still interact with the original content while more content is being loaded.

My ask is if you can make having the loading message replace the content entirely as optional. The way I would do that is by allowing the isLoadingMessage prop to also accept a boolean, and I'd even go further to suggest defaulting to false (off) just because I'm not sure that it's going to be the most common need. I left some code comments on how you could approach it.

@cchaos Thanks for the suggestion and for providing insight into a real use case. Here's where I took the example:

kibanamachine · 2020-08-12T22:07:38Z

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

cchaos

Sweet, thanks! Just some nits now

src-docs/src/views/accordion/accordion_example.js

cchaos · 2020-08-13T19:31:15Z

CHANGELOG.md

 ## [`master`](https://github.com/elastic/eui/tree/master)

-No public interface changes since `28.0.0`.
+- Add `isLoading` and `isLoadingMessage` props to `EuiAccordion` ([#3879](https://github.com/elastic/eui/pull/3879))


Suggested change

- Add `isLoading` and `isLoadingMessage` props to `EuiAccordion` ([#3879](https://github.com/elastic/eui/pull/3879))

- Added `isLoading` and `isLoadingMessage` props to `EuiAccordion` ([#3879](https://github.com/elastic/eui/pull/3879))

cchaos · 2020-08-13T19:32:50Z

src-docs/src/views/accordion/accordion_isLoading.js

+      <EuiFlexGroup alignItems="center">
+        <EuiFlexItem grow={false}>
+          <EuiTitle size="xs">
+            <h2>isLoadingMessage: </h2>


Suggested change

<h2>isLoadingMessage: </h2>

<h3>isLoadingMessage: </h3>

src/components/accordion/accordion.test.tsx

kibanamachine · 2020-08-13T22:35:06Z

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

kibanamachine · 2020-08-14T19:15:51Z

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

kibanamachine · 2020-08-17T04:05:47Z

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

cchaos

👍 Looks Great! Thanks for making it more flexible.

cchaos · 2020-08-17T15:14:37Z

src-docs/src/views/accordion/accordion_example.js

+  isLoading
+  isLoadingMessage={customMessage}
+  >
+  <!-- Content that will be replaced by isLoadingMessage -->


andreadelrio added 2 commits August 9, 2020 15:30

add isLoading and isLoadingMessage to EuiAccordion

170f955

add cl

05c49a7

andreadelrio force-pushed the isLoading-accordion branch from b3ca120 to 05c49a7 Compare August 9, 2020 22:30

andreadelrio requested a review from elizabetdev August 10, 2020 16:27

elizabetdev suggested changes Aug 11, 2020

View reviewed changes

andreadelrio added 2 commits August 11, 2020 17:46

pr feedback

485fd87

Merge remote-tracking branch 'upstream/master' into isLoading-accordion

7881007

remove space

654aa1b

andreadelrio force-pushed the isLoading-accordion branch from b19996f to 654aa1b Compare August 12, 2020 04:59

elizabetdev approved these changes Aug 12, 2020

View reviewed changes

src-docs/src/views/accordion/accordion_isLoading.js Outdated Show resolved Hide resolved

cchaos suggested changes Aug 12, 2020

View reviewed changes

andreadelrio added 2 commits August 12, 2020 14:22

more feedback

304e5fb

a11y

4f7c4c2

andreadelrio requested a review from cchaos August 12, 2020 21:51

cchaos suggested changes Aug 13, 2020

View reviewed changes

nits

34b1d2f

andreadelrio requested a review from cchaos August 14, 2020 18:23

Merge branch 'master' into isLoading-accordion

c36142d

andreadelrio added 2 commits August 16, 2020 18:34

cl and fix console error when using buttonGroup

d177cb4

cl

e45da9f

cchaos approved these changes Aug 17, 2020

View reviewed changes

andreadelrio merged commit 12823bf into elastic:master Aug 17, 2020

nyurik pushed a commit that referenced this pull request Aug 18, 2020

Add isLoading and isLoadingMessage props to EuiAccordion (#3879)

8dd4187

thompsongl mentioned this pull request Aug 21, 2020

Upgrade EUI to v28.2.0 elastic/kibana#75700

Merged

	- Add `isLoading` and `isLoadingMessage` props to `EuiAccordion` ([#3879](https://github.com/elastic/eui/pull/3879))
	- Added `isLoading` and `isLoadingMessage` props to `EuiAccordion` ([#3879](https://github.com/elastic/eui/pull/3879))

Conversation

andreadelrio commented Aug 9, 2020

Summary

Checklist

Uh oh!

kibanamachine commented Aug 9, 2020

Uh oh!

elizabetdev left a comment

Choose a reason for hiding this comment

Uh oh!

andreadelrio commented Aug 12, 2020

Uh oh!

kibanamachine commented Aug 12, 2020

Uh oh!

kibanamachine commented Aug 12, 2020

Uh oh!

kibanamachine commented Aug 12, 2020

Uh oh!

elizabetdev left a comment

Choose a reason for hiding this comment

Uh oh!

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!

cchaos Aug 12, 2020

Choose a reason for hiding this comment

Uh oh!

andreadelrio commented Aug 12, 2020

Uh oh!

kibanamachine commented Aug 12, 2020

Uh oh!

cchaos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cchaos Aug 13, 2020

Choose a reason for hiding this comment

Uh oh!

cchaos Aug 13, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kibanamachine commented Aug 13, 2020

Uh oh!

kibanamachine commented Aug 14, 2020

Uh oh!

kibanamachine commented Aug 17, 2020

Uh oh!

cchaos left a comment

Choose a reason for hiding this comment

Uh oh!

cchaos Aug 17, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants