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

add loading state to alerts #4480

Closed
wants to merge 1 commit into from

Conversation

nnchang
Copy link
Contributor

@nnchang nnchang commented Jan 13, 2021

I ran into a situation where I wanted to make an alert that had a clear load state (specifically the one supplied by <Button>). Because <Alert> doesn't give allow much control over the buttons, I wound up implementing that solution using a dialog and styling it to look like an alert. I think it would be nice for <Alert> to have this functionality.

Checklist

  • Includes tests
  • Update documentation

Changes proposed in this pull request:

Add a loading state to <Alert>.

Reviewers should focus on:

I'm not familiar with the testing library, so I haven't yet added tests. I can dig into that if maintainers find this to be an acceptable suggestion.

Screenshot

alert loading state

@nnchang
Copy link
Contributor Author

nnchang commented Jan 13, 2021

side note: as a new contributor, I was surprised that the yarn dev command as suggested in the CONTRIBUTING.md took me straight to the table app and didn't seem to have the core package building. Looking at package.json, I think this is expected behavior. I'm happy to make a PR to change the docs if appropriate.

@adidahiya
Copy link
Contributor

good point on yarn dev from root. I made a change in 842b2bb but forgot to reverse the negation in the glob. I'll fix that.

it looks like you may have enabled CircleCI for your fork, which is not encouraged -- we want your PR to build on our org's Circle instance to report status checks correctly.

as for the new prop itself... I am generally very conservative about what we add to Alert but in this case I think it's an elegant addition and I support it 👍

@nnchang
Copy link
Contributor Author

nnchang commented Jan 14, 2021

cool! I will close this PR and try to reopen such that circle will run correctly.

@nnchang nnchang closed this Jan 14, 2021
@nnchang nnchang mentioned this pull request Jan 14, 2021
2 tasks
This pull request was closed.
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