Skip to content
This repository was archived by the owner on May 29, 2019. It is now read-only.

Refactor progressbar #1564

Closed
wants to merge 1 commit into from
Closed

Refactor progressbar #1564

wants to merge 1 commit into from

Conversation

bekos
Copy link
Contributor

@bekos bekos commented Jan 12, 2014

No description provided.

@chrisirhc
Copy link
Contributor

Hmm.. When I mentioned an optional watcher, I was hoping to just give users the ability to write a directive like dynamicMax but not actually provide it in our codebase. We could instead have it in the wiki.

@bekos
Copy link
Contributor Author

bekos commented Jan 12, 2014

I think this is a common enough case to add into the library and there is no contradiction into it's usage. I believe it's worth the few extra bytes to have a complete module :-)

var progressCtrl = ctrls[0] || ctrls[1];

if ( !progressCtrl || !scope.$eval(attrs.dynamicMax) || !attrs.max ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It just worries me that this again, adds another evaluation and possibly causes some people to think that this itself is a dynamic attribute. However, this attribute is only evaluated once.

If we really need to have this directive, I suggest leaving out the evaluation and just adding this watcher on the presence of this directive (so the usage would just be <progress max="x" dynamic-max></progress>) .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see your point, but the problem is that for example in a ng-repeat you cannot mix dynamic progressbars with not. Also it's easier to specify it at runtime without the need to alter/compile the HTML.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand that use case but I'm worried that supporting such possibly rare use cases will bloat up the bootstrap library with additional watchers and directives and more lines of code.

I'm a little uneasy with supporting all these custom use cases that would add overhead for users that are just looking for something simple and works very well and is fast. When something is added on one component, users also expect it for other components. As such, one addition here means much more additions in the future. If we add another feature that's configurable in the future, we then have to consider whether to add a dynamic watcher for it and/or an option that enables the dynamic watcher.

However, I don't know how to measure if a use case is rare or common, so I'm quite at a loss for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that we don't know and cannot tell what is rare or not. The reason of adding this kind of directives is to keep the library fast and the organization of code simple. I disagree that the users that are using this library are looking for something simple and I am totally sure that many of them will not know or even bother to go to a wiki to search for a custom directive and manually add it to their application. Most of the times, they just pass and look for another alternative that supports it out of the box.

So, IMO this is another discussion we should take in the near future (cc: @pkozlowski-opensource ) about how far we want to go :-)

Now, in terms of consistency, I believe that all optional directives should be evaluated as boolean for the same reasons as above. If this directive makes the library bloated or unmaintainable I also agree to remove, but leaving out a feature for the shake of 15 LOC added is not possibly the best.

 * Remove the need to find out the old value.
 * Reorganize code.
@bekos bekos closed this in 22fc117 Jan 22, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants