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.

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

refactor: [M3-8183] - Clean up loading components #10524

Merged

Conversation

hana-akamai
Copy link
Contributor

@hana-akamai hana-akamai commented May 29, 2024

Description 📝

We have a lot of loading components that are all basically just variations of CircleProgress. This PR cleans up those components.

I also moved the overage pricing tooltip text position in the Object Storage Create Bucket drawer to the top so that the tooltip doesn't block the CTA buttons

Preview 📷

Before After
image image
image image
image image

How to test 🧪

Verification steps

(How to verify changes)

  • Check the affected loading spinners throughout the app

As an Author I have considered 🤔

Check all that apply

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • 🧪 Providing/Improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

@hana-akamai hana-akamai self-assigned this May 29, 2024
@@ -88,9 +87,6 @@ export const WrapperMenuItem = (props: WrapperMenuItemCombinedProps) => {
{...rest}
className={`${classes.root} ${className} ${tooltip && 'hasTooltip'}`}
>
{isLoading && (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This loading state was never used

@hana-akamai hana-akamai marked this pull request as ready for review May 29, 2024 16:38
@hana-akamai hana-akamai requested a review from a team as a code owner May 29, 2024 16:38
@hana-akamai hana-akamai requested review from bnussman-akamai and jaalah-akamai and removed request for a team May 29, 2024 16:38
Copy link

github-actions bot commented May 30, 2024

Coverage Report:
Base Coverage: 82.35%
Current Coverage: 82.3%

Copy link
Contributor

@abailly-akamai abailly-akamai left a comment

Choose a reason for hiding this comment

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

Nice cleanup! PR looks good. left some comments & suggestions

Copy link
Contributor

@abailly-akamai abailly-akamai left a comment

Choose a reason for hiding this comment

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

Great Cleanup! did not notice regressions through the flows. Thanks for looking at improving the size API ✨

One improvement remains IMO:

when we don't enforce a size, <CircleProgress /> has different treatment:
Screenshot 2024-06-04 at 09 16 46

It's larger and features an inner circle. it's a common loader since it is used for the splashscreen and and loading landing page
For the sake of addressing this globally and make the component's API clear (right now it's unclear NOT adding a prop renders the largest version), i think we should either:

  • require size, add a lg option and add it to all circle progress without a size prop (will increase PR size but it's a no regression change)
  • still add an lg option, and set it by default in the component.
  • for either option, i would suggest removing the inner circle cause i am not sure what it adds?

These are just suggestions, it's not a deal breaker to go along with this cleanup. Approving pending whatever solution you go with and others chiming in

@hana-akamai hana-akamai added the Add'tl Approval Needed Waiting on another approval! label Jun 4, 2024
Copy link
Member

@bnussman-akamai bnussman-akamai left a comment

Choose a reason for hiding this comment

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

I wonder if we could consider disableShrink . I think it looks slightly better, but I'm not sure

@bnussman-akamai
Copy link
Member

Can we move the disableShrink option into the theme?

Screenshot 2024-06-05 at 1 41 25 PM

Copy link
Member

@bnussman-akamai bnussman-akamai left a comment

Choose a reason for hiding this comment

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

Thanks! Nice clean up! 🧹

@abailly-akamai abailly-akamai removed the Add'tl Approval Needed Waiting on another approval! label Jun 5, 2024
@hana-akamai hana-akamai merged commit a0c4520 into linode:develop Jun 5, 2024
18 checks passed
@hana-akamai hana-akamai deleted the M3-8183-clean-up-loading-components branch June 5, 2024 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants