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

LG-4622: Adds a loading state to the Chart component #2652

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

tsck
Copy link
Collaborator

@tsck tsck commented Jan 22, 2025

✍️ Proposed changes

  • Adds a loading state to the Chart component
  • Designs

🎟 Jira ticket: LG-4622

✅ Checklist

For bug fixes, new features & breaking changes

  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • I have run pnpm changeset and documented my changes

For new components

  • I have added my new package to the global tsconfig
  • I have added my new package to the Table of Contents on the global README
  • I have verified the Live Example looks as intended on the design website.

🧪 How to test changes

Test via isLoading prop in storybook

Screenshot

Screenshot 2025-01-22 at 12 00 35 PM

Copy link

changeset-bot bot commented Jan 22, 2025

🦋 Changeset detected

Latest commit: fc8e9e0

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@lg-charts/core Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@tsck tsck marked this pull request as ready for review January 22, 2025 17:31
@tsck tsck requested a review from a team as a code owner January 22, 2025 17:31
@tsck tsck requested review from stephl3 and removed request for a team January 22, 2025 17:31
Comment on lines 40 to 43
/**
* Controlled loading state.
*/
isLoading?: boolean;
Copy link
Collaborator

Choose a reason for hiding this comment

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

is there any possibility we might want to render an error if chart data doesn't load or encounters some other issue? if so, I'm curious what you think about using a more extensible enum rather than a boolean for state

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a very good callout. Internal Tools had mentioned an error state as well and I meant to talk to Sooa about this. There's nothing in the designs for this but there probably should be. I will definitely update it so that there's an enum rather than a boolean for this case, and I'll ping design about the error state (though will probably add that separately, assuming design wants it)

Comment on lines +17 to +21
export const ChartStates = {
Default: 'default',
Loading: 'loading',
} as const;
export type ChartStates = (typeof ChartStates)[keyof typeof ChartStates];
Copy link
Collaborator

Choose a reason for hiding this comment

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

We also have loading/error states in Combobox and SearchInput and those use Unset: 'unset' instead of Default: 'default'. Can we use the same for consistency?

export const SearchState = {
Unset: 'unset',
Error: 'error',
Loading: 'loading',
} as const;
export type SearchState = (typeof SearchState)[keyof typeof SearchState];

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants