Skip to content

feat(progress): adds progress bar #12010

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

Closed

Conversation

abdel-ships-it
Copy link

@abdel-ships-it abdel-ships-it commented Jun 11, 2017

Short description of what this resolves:

This resolves #6669

Changes proposed in this pull request:

  • Add progress bars with determinate, indeterminate, buffer and query as available modes
  • Theming options for the progress bar
  • RTL support
LTR RTL

This PR has some SCSS linting errors, I hope @brandyscarney can help out here :)

Ionic Version: 2.x

@abdel-ships-it abdel-ships-it changed the title Feature/progress bar feat(progress) Jun 11, 2017
@abdel-ships-it abdel-ships-it changed the title feat(progress) feat(progress): adds progress bar Jun 11, 2017
@AmitMY
Copy link
Contributor

AmitMY commented Jun 12, 2017

This looks visually great, both LTR and RTL.
Please run gulp lint as I can spot some scss linting issues.

I wonder though, is progress bar the same on all platforms? (ios/md/wp) if not, there should be progress.md.scss / progress.wp.scss / progress.ios.scss files

// --------------------------------------------------

/// @prop - The color of the element that is being animated
$progress-color:rgb(16,108,200) !default;
Copy link
Contributor

Choose a reason for hiding this comment

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

Something lint won't catch, is variables alignment. Please keep all variable values aligned at the same column

background-color: $progress-color;
transition: width $progress-values-transition cubic-bezier(0.4,0.0,0.2,1);
}
.bufferCircles {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure if linting will catch this, but it should be buffer-circles

Copy link
Member

Choose a reason for hiding this comment

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

Agree with @AmitMY here, please use kebab-case for CSS classes.

width: 100%;

display: none;
mask: url("data:image/svg+xml;base64,PD94bWwgdmVyc2lvbj0iMS4wIj8+Cjxzdmcgd2lkdGg9IjEyIiBoZWlnaHQ9IjQiIHZpZXdQb3J0PSIwIDAgMTIgNCIgdmVyc2lvbj0iMS4xIiB4bWxucz0iaHR0cDovL3d3dy53My5vcmcvMjAwMC9zdmciPgogIDxlbGxpcHNlIGN4PSIyIiBjeT0iMiIgcng9IjIiIHJ5PSIyIj4KICAgIDxhbmltYXRlIGF0dHJpYnV0ZU5hbWU9ImN4IiBmcm9tPSIyIiB0bz0iLTEwIiBkdXI9IjAuNnMiIHJlcGVhdENvdW50PSJpbmRlZmluaXRlIiAvPgogIDwvZWxsaXBzZT4KICA8ZWxsaXBzZSBjeD0iMTQiIGN5PSIyIiByeD0iMiIgcnk9IjIiIGNsYXNzPSJsb2FkZXIiPgogICAgPGFuaW1hdGUgYXR0cmlidXRlTmFtZT0iY3giIGZyb209IjE0IiB0bz0iMiIgZHVyPSIwLjZzIiByZXBlYXRDb3VudD0iaW5kZWZpbml0ZSIgLz4KICA8L2VsbGlwc2U+Cjwvc3ZnPgo=");
Copy link
Contributor

Choose a reason for hiding this comment

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

If you can use svg-background-image instead of mask, please do.
If not, can you do:
`mask: url("data:image/svg+xml;charset=utf-8,#{url-encode($progress-mask-svg)}")``

Where $progress-mask-svg is your svg, or a scalable version of it like:

<svg xmlns='http://www.w3.org/2000/svg' viewBox="0 0 12 4">
  <ellipse cx="2" cy="2" rx="2" ry="2">
    <animate attributeName="cx" from="2" to="-10" dur="0.6s" repeatCount="indefinite" />
  </ellipse>
  <ellipse cx="14" cy="2" rx="2" ry="2" class="loader">
    <animate attributeName="cx" from="14" to="2" dur="0.6s" repeatCount="indefinite" />
  </ellipse>
</svg>


@Component({
selector: 'ion-progress',
templateUrl : './progress.html',
Copy link
Contributor

Choose a reason for hiding this comment

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

instead, use template and put your template inline, like the other components

constructor( ) { }

/**
* @input {'determinate' | 'indeterminate' | 'buffer' | 'query'} The current mode of the progress indicator
Copy link
Contributor

Choose a reason for hiding this comment

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

add (above the class): export type progressIndicator = 'determinate' | 'indeterminate' | 'buffer' | 'query' and use it instead of explicit strings

Copy link
Member

Choose a reason for hiding this comment

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

@AmitMY I believe query is both determinate and indeterminate. It's been awhile since I looked at these but if I recall correctly it displays as indeterminate during the call for data and then determinate once it gets the data and is still loading. See here:

https://material.io/guidelines/components/progress-activity.html#progress-activity-types-of-indicators

I like using the name query, it's short and goes with the MD guidelines. Maybe we just need to document what each indicator does really well.

* @input {'determinate' | 'indeterminate' | 'buffer' | 'query'} The current mode of the progress indicator
*/
@Input()
@HostBinding('class')
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of using HostBinding, usually they use host on Component, see range

Copy link
Author

Choose a reason for hiding this comment

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

According to the angular docs its best to use Hostbinding in contrast with host. But if you would like me to use host instead, I have no problem in doing so.

Copy link
Contributor

Choose a reason for hiding this comment

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

host is used across the framework. Perhaps the team can change it in v4, but I think it is best to keep everything to the same standard. @brandyscarney what is your take on this?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm yeah can we use host please just to keep it consistent? Also with v4 we will most likely end up using host anyways so it will be easier to port the component. 🙂

Copy link
Author

Choose a reason for hiding this comment

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

Alright! No problems, consistency is definitely important.

@AmitMY
Copy link
Contributor

AmitMY commented Jun 12, 2017

  • To your basic test, please add an e2e.ts file (even an empty one)
  • To your test folder, please add progress.spec.ts, and write a cupple of simple tests, just so we always know it functions correctly (for example, changing indicator adds correct class. (see button.spec.ts)

RTL support is not implemented fully correctly, but that is because I introduced something new to flipping components here - #12004. When that is merged, I'll give you tips on how to improve on what you already did. (it works, but we have a mixin for that)

@abdel-ships-it
Copy link
Author

I have two tests file locally that just need to be committed, whats the test command for running e2e tests and unit tests though?

And I have a strange feeling you are referring to the rtl mixing​ which I was told to use in the issue.

@AmitMY
Copy link
Contributor

AmitMY commented Jun 12, 2017

@realappie To run unit tests run gulp validate
I am not sure how to run the e2e file with protractor, only in snapshot. perhaps @brandyscarney can help.
To run it in browser do gulp e2e.watch --f=progress/basic

About RTL, yeah, you did correctly in general, I just meant it can be refined. I will give more feedback after linting fixes are done.

0% {
width: 0%;

left: 0%;
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of left, use:

@include multi-dir() {
  // scss-lint:disable PropertySpelling
  left: 0%;
}

same for all keyframes.

*It is needed for if any class wants to override it

Copy link
Author

Choose a reason for hiding this comment

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

Alright I will incorporate that :)

Copy link
Author

Choose a reason for hiding this comment

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

This breaks my animation :/

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough, makes sense, can you just add that comment then? Otherwise it won't lint.

@manucorporat
Copy link
Contributor

manucorporat commented Jun 12, 2017

@realappie this looks awesome! and the feedback from @AmitMY is absolutely correct.

One thing we need to consider before merging this, is the design of the progress bar for iOS and WP, maybe @bensperry can help here.

Also, I think ion-progress should inherit from Ion base class, just like other components, this way we can support [color] and styling based in the mode.

@manucorporat
Copy link
Contributor

Take the simple fab component as an example:
https://github.com/ionic-team/ionic/blob/master/src/components/fab/fab.ts

Also, notice we avoid using ion-fab (or ion-progress in this case) in the scss, but we rely in the classes added by Ion.

https://github.com/ionic-team/ionic/blob/master/src/components/fab/fab.scss#L22

@@ -0,0 +1,4 @@
<div class="progress" [style.width.%]="value"></div>
<div class="animtable"></div>
Copy link
Contributor

Choose a reason for hiding this comment

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

typo?

Copy link
Author

Choose a reason for hiding this comment

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

I think I made the same typos multiple times ops, its supposed to be animatable rather than animtable

@brandyscarney
Copy link
Member

This looks great! Great work. 😄 I responded to a few comments with some feedback. @AmitMY has left some good feedback, as well.

@manucorporat I spoke to Ben about this awhile ago when I first started looking into it, and I was instructed to get Material Design looking right and match that for the other modes. We can always tweak styling of other modes later.

@abdel-ships-it
Copy link
Author

Thanks for your valuable feedback everyone, I will be improving the the component.

@brandyscarney How do I run e2e tests? Not the ones in the browser but the ones in the e2e.ts file.

@brandyscarney
Copy link
Member

@realappie The only way we use those files is to include the tests in our snapshot command (I wrote a blog on this here). I usually run the following commands:

gulp e2e.prod --f=progress
gulp snapshot.skipBuild --f=progress

Although you need the snapshot key and a retina screen in order to run it accurately: https://github.com/ionic-team/ionic/blob/master/scripts/README.md#running-snapshot

Are you in our worldwide slack group? https://ionicworldwide.herokuapp.com/

@abdel-ships-it
Copy link
Author

Little update on the progress bar, I have been very busy with other things the past two weeks and will have it finished this week :)

@abdel-ships-it
Copy link
Author

@brandyscarney I want to write some tests in karma, how do I run those tests only for the progress component?

$progress-color: rgb(16,108,200) !default;

/// @prop - The background color of the progress bar
$progress-buffer-color: rgb(170,209,249) !default;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please align the value and the value above to the same column as the value below?

Copy link
Author

Choose a reason for hiding this comment

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

The values are aligned? Or do you want me to align the word default as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean the entire variables alignment. This is how I see it (rgb is like 2 spaces before the others)
image

Copy link
Author

Choose a reason for hiding this comment

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

Oh yes, you're right. It seemed on the same line because of vscode's color preview 😂. It renders this block on the left of each valid CSS color.

Copy link
Contributor

Choose a reason for hiding this comment

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

Haha Microsoft strikes again :P

// --------------------------------------------------

/// @prop - The color of the element that is being animated
$progress-color: rgb(16, 108, 2001) !default;
Copy link
Member

Choose a reason for hiding this comment

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

I think there was a 1 accidentally added at the end here.

Copy link
Author

Choose a reason for hiding this comment

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

You are right.

Copy link
Member

Choose a reason for hiding this comment

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

Could you update this and then after some testing we could get it merged? Is it ready?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is currently material only, and there is no separation of .scss and .md.scss, which I at least feel is needed for it not to be changed after being merged (all the classes names)

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm, yes you are right. It would probably be good to at least make the progress.mode.scss files for determining the color of the progress bar per mode, and then using our color functions for the primary and buffer color.

In order for this to work though we need to change some things in the Progress class so that the color classes will be added. This causes other issues because of the progress class in the template. I'm thinking we should rename the template classes to be more specific (also makes it less likely to have conflicts with user's classes).

I created a commit for these changes here: cfb3972

Naming is up for discussion if any of them don't make sense, but it reduces nesting as well in the Sass files. I'm sure some improvements can be made there, let me know your thoughts.

@realappie Are you on Slack? If you send me or comment your email I could invite you to ours for discussion.

@skyserpent
Copy link

Hey guys, how come this is taking so long when the PR is already created? Can we get remaining issues fixed soon? I am waiting on this feature. I really appreciate the work and hope to see this merged soon! 💯

@lordgreg
Copy link

lordgreg commented Aug 5, 2017

I'm also really interested to see the progress bars being finally added to ionic. As it seems, everything is done and ready to be merged?

@brandyscarney
Copy link
Member

Just wanted to update everyone on why some PR's haven't been merged. We've been hard at work on the mono-refactor branch which is eventually going to turn into our v4 release. There are a lot of changes coming that will have major performance improvements and fix some longstanding issues. We've touched on this in the "What's next" section of our blog post here.

With v4 comes some big changes internally, so every change done to master can't be simply pulled to the branch, it has to be manually ported. Unfortunately, this has caused us to slow down on merging some changes to master for now. I do want to get this PR merged though before the v4 release. I added some comments above with some feedback and a commit of some partial changes that should be implemented to get it working with colors & modes.

@abdel-ships-it
Copy link
Author

@brandyscarney Good working on cfb3972. I have actually been on slack for a while now under realappie I messaged you about the color customisation a few weeks ago.

@abdel-ships-it
Copy link
Author

@brandyscarney Are you already migrating all the ionic 3.x components to stencil branch internally? Or should I pick that up.

@AmitMY
Copy link
Contributor

AmitMY commented Sep 4, 2017

@abdel-ships-it
Copy link
Author

@AmitMY Is that a yes or no? :P And do you have any slack channel for contributors?

@AmitMY
Copy link
Contributor

AmitMY commented Sep 4, 2017

Yep, they are already migrating the ionic components to stencil in that branch.
There is a channel, I think @adamdbradley is the owner of it, if you want to be added.

@brandyscarney
Copy link
Member

Sorry, yes @realappie we have started porting components. I sent you a message on the worldwide slack. If you could send me your email there or post it here (I can delete the comment afterwards) I can add you to our company slack for discussion. Thanks! 😄

@brandyscarney brandyscarney added this to the 3.7.0 milestone Sep 13, 2017
@brandyscarney brandyscarney modified the milestones: 3.7.0, 3.8.0 Sep 29, 2017
@mburger81
Copy link
Contributor

Perhaps this could be added for 3.9.0?

@kensodemann kensodemann modified the milestones: 3.8.0, 3.planning Oct 27, 2017
@kensodemann
Copy link
Member

I moved it into 3.planning since 3.7.2 became 3.8.0 and we already planned 3.9.0 without this, so if someone has the time it may get into 3.9.0 or maybe not but at least it will be in a bucket of things to look at first.

@kensodemann kensodemann removed this from the 3.planning milestone Nov 30, 2017
@abdel-ships-it
Copy link
Author

@kensodemann I will see if I can re-write it :) Should I try and get it into 3.9 or just rewrite it in the stencil branch? Whats the best thing to do here.

@AmitMY
Copy link
Contributor

AmitMY commented Dec 4, 2017

@realappie Nothing official, but the way I see it, new stuff were not merged into master for many moons now, and only the core one is actually moving..

@kensodemann kensodemann added this to the 4.x milestone Dec 4, 2017
@kensodemann
Copy link
Member

@realappie - I changed the milestone to 4.x

My suggestion is that you see if you can target v4 and the core branch. At this point, I do not expect there to be another v3.x release.

@abdel-ships-it
Copy link
Author

@kensodemann I wouldn't mind another 3.x release, I think building for 4.x would be the best thing to do at the moment. I will see if I can make some time for it this weekend!

@brandyscarney
Copy link
Member

brandyscarney commented Mar 12, 2018

We updated the core branch (with v4) to master today which broke this PR. I am going to close this since there are conflicts now, but I would really love to get this feature added in v4. Please reach out to me on Slack (either our team slack or worldwide) or by email (brandy at ionic dot io). I will help out as much as possible if this is something you would like to port over. Thanks for your time!

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.

Feature: Add Progress bars
10 participants