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(progress-bar): add Progress Bar #16559

Merged
merged 67 commits into from
Dec 10, 2018

Conversation

paulstelzer
Copy link
Contributor

@paulstelzer paulstelzer commented Dec 3, 2018

Short description of what this resolves:

Adds a progress bar component to Ionic 4. Animation is based on MD spec (https://material.io/design/components/progress-indicators.html#linear-progress-indicators )

Feature Request: #16558

BASIC

2018-12-09_10-25-36

Preview

2018-12-09_10-26-16

@ionitron-bot ionitron-bot bot added the package: core @ionic/core package label Dec 3, 2018
@manucorporat
Copy link
Contributor

I like it a lot! we could definitely merge it after 4.0 final is released!

We try to avoid nested SCSS rules, could you try to flatten that to at most 1 level nested?

@paulstelzer paulstelzer changed the title feat(progress-bar): Add Progress Bar [WIP] feat(progress-bar): Add Progress Bar [need check] Dec 3, 2018
@paulstelzer paulstelzer changed the title feat(progress-bar): Add Progress Bar [need check] feat(progress-bar): Add Progress Bar [need check and help] Dec 3, 2018
@brandyscarney
Copy link
Member

Very nice! I'll review this soon.

@brandyscarney
Copy link
Member

This is looking really good! I added some comments for places the docs could improve but it isn't critical. 🙂

My only concern with the API is using type, do you think we will have issues with this interfering with native? @manucorporat @adamdbradley. I believe we were calling it indicator in the v3 one.

@manucorporat
Copy link
Contributor

manucorporat commented Dec 10, 2018

@brandyscarney I like type because it's the "obvious" API, easier to remember imo.
We also use type in other components: ripple-effect, menu

screenshot 2018-12-10 at 18 26 26

While type is a property of button and input, it's not a global HTML attribute.

@paulstelzer
Copy link
Contributor Author

@brandyscarney Thanks for your review! What about the css variables? @manucorporat and I discussed a lot of it yesterday, at the end, this is my favorite (it's not in this PR already):

* @prop --progress-bar-color: Color of the progress bar (foreground)
* @prop --buffer-bar-color: Color of the buffer bar (background)
* @prop --buffer-circle-color: Color of the buffer circles

@manucorporat
Copy link
Contributor

@paulstelzer @brandyscarney I would:

  1. remove --buffer-circle-color: start with a small API, does not seem like a common use case, we can add it later
  2. Add --color, and --background:
  --color: #{ion-color(primary, base)};
  --background: #{ion-color(primary, base, 0.2)};
  --progress-bar-background: var(--color);
  --buffer-bar-background: var(--background);

@brandyscarney
Copy link
Member

I think we should keep it simple like the following:

--background: #{ion-color(primary, base, 0.2)};

--progress-background:  #{ion-color(primary, base)};
--buffer-background: var(--background);

which would allow for things like this to come in later:

--progress-background-disabled
--buffer-background-disabled

Also, avoid using bar since material design also has a circular indicator and I think these could be shared with it.

@manucorporat manucorporat changed the title feat(progress-bar): Add Progress Bar [need check and help] feat(progress-bar): add Progress Bar Dec 10, 2018
Copy link
Contributor

@manucorporat manucorporat left a comment

Choose a reason for hiding this comment

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

LTGM!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: core @ionic/core package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants