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

feat: add loading indicator #857

Merged
merged 5 commits into from
Aug 6, 2020
Merged

feat: add loading indicator #857

merged 5 commits into from
Aug 6, 2020

Conversation

ruphin
Copy link
Collaborator

@ruphin ruphin commented Aug 4, 2020

This adds a loading indicator package. There are some things I'm not super satisfied with, so please review carefully and check my notes.

@changeset-bot
Copy link

changeset-bot bot commented Aug 4, 2020

🦋 Changeset is good to go

Latest commit: ea66e40

We got this.

This PR includes changesets to release 1 package
Name Type
@lion/progress-indicator 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

@CLAassistant
Copy link

CLAassistant commented Aug 4, 2020

CLA assistant check
All committers have signed the CLA.

packages/progress-indicator/src/LionProgressIndicator.js Outdated Show resolved Hide resolved
packages/progress-indicator/src/LionProgressIndicator.js Outdated Show resolved Hide resolved
packages/progress-indicator/src/LionProgressIndicator.js Outdated Show resolved Hide resolved
packages/progress-indicator/src/LionProgressIndicator.js Outdated Show resolved Hide resolved
@@ -0,0 +1,3 @@
export default {
loading: 'Loading',
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The accessibility readers will see this component as "Loading", which is probably correct in nearly all cases, but there may be places where a progress indicator is used that does not indicate "Loading" but something else. Do we want to make it possible to override this?

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense. We could consider calling a method _setLabel() inside onLocaleUpdated, so Sub classers can override this

@ruphin
Copy link
Collaborator Author

ruphin commented Aug 4, 2020

TODO: Add a changeset to this pull request

@ruphin ruphin marked this pull request as ready for review August 5, 2020 16:06
@ruphin
Copy link
Collaborator Author

ruphin commented Aug 5, 2020

I added a changeset to release version 0.1.0 of this. I think all comments have been addressed.

The tests are failing because test coverage is not high enough, although I don't think it's caused by this package. The tests for this are passing.

Feel free to merge if it passes review.

@ruphin ruphin merged commit c224baa into master Aug 6, 2020
@ruphin ruphin deleted the loading_indicator branch August 6, 2020 11:35
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.

6 participants