Skip to content
This repository has been archived by the owner on Dec 1, 2023. It is now read-only.

Added a progress bar directive, with tests and docs #1389

Closed
wants to merge 3 commits into from

Conversation

DaveWM
Copy link

@DaveWM DaveWM commented Dec 30, 2014

resolves #379 , adds a progress bar directive. Allows you to enable/disable animations and select the type of progress bar (success, warning, etc.).

@mgcrea
Copy link
Owner

mgcrea commented Feb 14, 2015

Thanks for the PR, but I can't review it as you've committed the build files, please remove them and squash your commits into a single one with a proper message format (check the guidelines).

add a directive for a progress bar, with tests and docs

resolve mgcrea#379
@DaveWM
Copy link
Author

DaveWM commented Feb 15, 2015

@mgcrea Done, hadn't seen the contributing guidelines sorry

@manikantag
Copy link

Does this allows using progress bar along with buttons? ex: kind of https://github.com/remotty/angular-ladda progress. Not exactly like this module, but will this allows adding progress bar to any other directive too?

Thanks.

@DaveWM
Copy link
Author

DaveWM commented Mar 13, 2015

@manikantag The progress bar directive compiles to a div, so it should work within a button yes

@manikantag
Copy link

oh, good then.

@vmlf01
Copy link
Collaborator

vmlf01 commented Apr 16, 2015

@mgcrea This looks good, should it be merged?

@mgcrea
Copy link
Owner

mgcrea commented Apr 17, 2015

@vmlf01, I'd like to merge this, but it feels like it needs some tweaks to be better aligned with the rest of the library (mostly scope handling & var naming). Needs a bit more investigation when I'll have the time.

transclude: true,
replace: true,
templateUrl: 'progressbar/progressbar.tpl.html',
scope:{
Copy link
Owner

Choose a reason for hiding this comment

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

AngularStrap uses scope: true accross the project, why use & for animate (and @ for type)?

Copy link
Author

Choose a reason for hiding this comment

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

I was using & for animate because I didn't know optional 2 way bindings existed, I've updated it to =? :). What's the advantage of using scope:true?

Copy link
Owner

Choose a reason for hiding this comment

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

In the past, the isolated scopes were buggy, and leaked to other directives (see angular/angular.js/issues/1924). So AngularStrap avoided them since they were mostly decorator directives and this bug broke ngModel bindings.
It is now fixed but the project has not migrated yet, for the sake of consistency, I'd rather keep defaulting to scope:true (leaving the isolated scope definition in comments to ease a future update). At some point (maybe for the next minor), we should migrate all directives at once.

@henry74
Copy link

henry74 commented Apr 28, 2015

Is this available in the latest build?

@vmlf01
Copy link
Collaborator

vmlf01 commented Apr 28, 2015

No, it hasn't been merged yet

@rzschech
Copy link

It would be nice if this change:

  • supported min/max not being 0/100 and computes the width percentage based on the min and max.
  • supported min-width as per the Bootstrap progress bar examples
  • the output {{value}}% span is optional

@DaveWM
Copy link
Author

DaveWM commented May 12, 2015

@rzschech

  • I did consider allowing the min/max values to be changed, but I think it's simpler to just convert the value to a percentage. Having said that, if you really need this feature I can add it in.
  • You can set the min-width via css:
.progress-bar{
    min-width: 2em;
}
  • The content within the progress bar is already optional. You can add whatever you like in here, like so:
<bs-progressbar value="value">My Content</bs-progressbar>

@mgcrea
Copy link
Owner

mgcrea commented May 20, 2015

Regarding the naming, I'm wondering if the naming shouldn't be bs-progress-bar & mgcrea.strap.progressBar, looks consistent with the official bootstrap naming.

@DeividasK
Copy link

Can't wait to see this PR merged.

@hermansje
Copy link
Contributor

Hopefully this can be merged soon!

@bjaraujo
Copy link

bjaraujo commented Jun 4, 2016

+1.

@JoelParke
Copy link

+1

@stale
Copy link

stale bot commented Jan 25, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jan 25, 2019
@stale stale bot closed this Feb 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Progressbar
10 participants