Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(LoadingIndicator): extract and use SVG animation directly #1540

Merged
merged 1 commit into from
Mar 10, 2023
Merged

feat(LoadingIndicator): extract and use SVG animation directly #1540

merged 1 commit into from
Mar 10, 2023

Conversation

booc0mtaco
Copy link
Contributor

Summary:

  • we only have one particular loading indicator we want to use at this time
  • apply styles and parameters to match previous implementation's output

Test Plan:

  • Wrote automated tests
  • CI tests / new tests are not applicable
  • Manually tested my changes, but I want to keep the details secret
  • Manually tested my changes, and here are the details:
    • update the snapshots as needed. verify results are otherwise the same
    • verify all existing tests pass

@booc0mtaco
feat(LoadingIndicator): extract and use SVG animation directly
0b15259 
- we only have one particular loading indicator we want to use at this time
- apply styles and parameters to match previous implementation's output
@booc0mtaco booc0mtaco requested a review from a team March 10, 2023 20:08
@github-actions
Copy link

size-limit report 📦

Path Size
components 121.45 KB (+0.29% 🔺)
styles 3.1 KB (0%)

@codecov
Copy link

codecov bot commented Mar 10, 2023 •
edited
Loading

Codecov Report

Merging #1540 (0b15259) into next (8c8018b) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             next    #1540   +/-   ##
=======================================
  Coverage   91.87%   91.87%           
=======================================
  Files         285      285           
  Lines        4293     4294    +1     
  Branches      791      792    +1     
=======================================
+ Hits         3944     3945    +1     
  Misses        324      324           
  Partials       25       25
Impacted Files Coverage Δ
...c/components/LoadingIndicator/LoadingIndicator.tsx 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

ahuth
ahuth approved these changes Mar 10, 2023
View reviewed changes
Copy link
Contributor

@ahuth ahuth left a comment

Choose a reason for hiding this comment

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

Love it 🤘

@booc0mtaco
Copy link
Contributor Author

@ahuth something is still depending on styled-components looks like. kinda weird...

@jinlee93
Copy link
Contributor

jinlee93 commented Mar 10, 2023 •
edited
Loading

@ahuth something is still depending on styled-components looks like. kinda weird...

Looks like the storybook-addon-badges package
image
But it looks like with its latest release no longer uses styled-components

EDS bump: #1541

jinlee93
jinlee93 reviewed Mar 10, 2023
View reviewed changes
src/components/LoadingIndicator/LoadingIndicator.tsx
*/
ariaLabel?: string;

/**
* Whether the oval is visible. Default is true.
* (Shadowed from OvalProps)
*
* See: https://mhnpd.github.io/react-loader-spinner/docs/components/oval
*/
visible?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the point of having an invisible loading spinner?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

visible for loaders allows for those cases where you want to preserve content when the loading indicator would otherwise be added and removed from the UI in quick succession. We can use this prop to handle transitions or other behaviors

@jinlee93 jinlee93 mentioned this pull request Mar 10, 2023
Merged
4 tasks
jinlee93
jinlee93 approved these changes Mar 10, 2023
View reviewed changes
Copy link
Contributor

@jinlee93 jinlee93 left a comment

Choose a reason for hiding this comment

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

Thanks!

@booc0mtaco booc0mtaco merged commit 6e315ea into next Mar 10, 2023
@booc0mtaco booc0mtaco deleted the aholloway/EDS-847 branch March 10, 2023 22:30
@booc0mtaco booc0mtaco mentioned this pull request Mar 17, 2023
Merged
booc0mtaco added a commit that referenced this pull request Mar 17, 2023
@booc0mtaco
Merge pull request #1549 from chanzuckerberg/release-v11.0.0
b52eace 
## [11.0.0](v10.0.0...v11.0.0) (2023-03-17)


### ⚠ BREAKING CHANGES

* add `indeterminate` prop to <Checkbox> that's separate from `checked` (#1520)

### Features

* add `indeterminate` prop to <Checkbox> that's separate from `checked` ([#1520](#1520)) ([d8e2cc4](d8e2cc4))
* **LoadingIndicator:** extract and use SVG animation directly ([#1540](#1540)) ([6e315ea](6e315ea))
* **menu:** add Menu.PlainButton as a minimally styled Menu button ([#1516](#1516)) ([8268d8e](8268d8e))


### Bug Fixes

* actually use our shared prettier config ([c98ea51](c98ea51))
* **Avatar:** loosen props for avatar aria-label component ([#1544](#1544)) ([4ab9183](4ab9183))
* markdown story styling ([#1536](#1536)) ([89eba6b](89eba6b))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jinlee93 jinlee93 jinlee93 approved these changes

@ahuth ahuth ahuth approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@booc0mtaco @jinlee93 @ahuth