Skip to content

Simplying EuiAccordion isLoading and isLoadingMessage logic#5896

Merged
1Copenut merged 6 commits intoelastic:mainfrom
1Copenut:feature/tpierce-accordion-isLoadingUpdate
May 24, 2022
Merged

Simplying EuiAccordion isLoading and isLoadingMessage logic#5896
1Copenut merged 6 commits intoelastic:mainfrom
1Copenut:feature/tpierce-accordion-isLoadingUpdate

Conversation

@1Copenut
Copy link
Copy Markdown
Contributor

@1Copenut 1Copenut commented May 11, 2022 •
edited
Loading

Summary

@cchaos requested a simpler isLoading and isLoadingMessage logic in our EuiAccordion component. The state handling has been simplified in this way: requested the loading spinner to show on closed accordions even if we do not have an extraAction defined. This PR captures that new logic.

  • If no isLoading or extraActions props are passed or are not true, the optionalActions DIV is not rendered.
  • When I toggle isLoading to true, the loading spinner appears on the right of closed and open accordions
  • When I toggle isLoading to true, extraActions will be hidden and the loading spinner shown
  • When I toggle isLoading to false the extraActions will be shown

UPDATE: The more I look at this, we're not replacing anything, just modifying the logic for showing a spinner. I'm going to remove the breaking change label unless reviewers think we need it. This PR is potentially a breaking change and closes #5893.

This PR will be taken out of draft when #5826 is merged and can be folded into this code change.

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

Simplying EuiAccordion isLoading and isLoadingMessage logic
7c4b00b 
* Simplifying isLoading logic to show the spinner.
* Updating one snapshot test.
* Adding last updates for screen reader, defaults, and TS types.
* Updated four snapshot tests.
@1Copenut 1Copenut added feature request breaking change PRs with breaking changes. (Don't delete - used for automation) labels May 11, 2022
@1Copenut 1Copenut requested a review from cchaos May 11, 2022 19:39
@1Copenut 1Copenut self-assigned this May 11, 2022
@kibanamachine
Copy link
Copy Markdown

kibanamachine commented May 11, 2022

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

@cchaos
Copy link
Copy Markdown
Contributor

cchaos commented May 16, 2022

@1Copenut What's the potential breaking change here? I see the update in logic being more of a bug fix since it should never have been tied directly to the isLoadingMessage.

@1Copenut
Copy link
Copy Markdown
Contributor Author

1Copenut commented May 16, 2022 •
edited
Loading

@cchaos The isLoadingMessage prop default being changed from "false" to "true" on line 110 is the item in question. I changed the default so I could simplify the logic to two cases: isLoadingMessage is either "true" or a string.

I felt like this new assumed default could cause issues upstream.

@cchaos
Copy link
Copy Markdown
Contributor

cchaos commented May 16, 2022

Oh, I think there was miscommunication about which spinner should be on by default when isLoading is true. It's not the accordion contents that should change, but the extraAction.

Right now, if you simply apply isLoading to a basic instance absolutely nothing shows up:
CleanShot 2022-05-16 at 15 24 13@2x

Only if you already passed content in the extraAction prop does the spinner show up
CleanShot 2022-05-16 at 15 25 39@2x

Instead, what we need to be doing is showing that spinner on the right whether or not extraAction exists.

The loading message should still be false by default because it's the least necessary/used part of indicating loading state and because it can cause large shifts in layout.

@1Copenut 1Copenut removed the breaking change PRs with breaking changes. (Don't delete - used for automation) label May 23, 2022
@1Copenut 1Copenut marked this pull request as ready for review May 23, 2022 21:10
@kibanamachine
Copy link
Copy Markdown

kibanamachine commented May 23, 2022

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

cchaos
cchaos approved these changes May 24, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Perfect, thank you!

@1Copenut @cchaos
Update upcoming_changelogs/5896.md
03d5701 
Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
@kibanamachine
Copy link
Copy Markdown

kibanamachine commented May 24, 2022

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

@1Copenut 1Copenut merged commit a0c8121 into elastic:main May 24, 2022
@1Copenut 1Copenut deleted the feature/tpierce-accordion-isLoadingUpdate branch May 24, 2022 19:09
cee-chen pushed a commit to cee-chen/eui that referenced this pull request May 26, 2022
@1Copenut @cchaos @cee-chen
Simplying EuiAccordion isLoading and isLoadingMessage logic (elastic#…
3910807 
…5896)

* Simplying EuiAccordion isLoading and isLoadingMessage logic

* Simplifying isLoading logic to show the spinner.
* Updating one snapshot test.
* Adding last updates for screen reader, defaults, and TS types.
* Updated four snapshot tests.

* WIP. Starting to refactor to show loading spinner without extra actions prop.

* Refactored logic to show closed accordion spinner on isLoading.

* Adding a changelog entry.

* Update upcoming_changelogs/5896.md

Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>

Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
@thompsongl thompsongl mentioned this pull request Jun 8, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@cchaos cchaos cchaos approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@1Copenut 1Copenut

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

3 participants

@1Copenut @kibanamachine @cchaos