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

feat(collapse): add collapsing/collapsed/expanding/expanded callbacks #5226

Closed
wants to merge 1 commit into from

Conversation

nonplus
Copy link
Contributor

@nonplus nonplus commented Jan 13, 2016

Closes #5194

@icfantv
Copy link
Contributor

icfantv commented Jan 13, 2016

@nonplus, thank you for taking the initiative to do this. Due to the changes that aren't actually changes it's really hard to see what's changed and what has not. Please fix your commit such that only the lines you've changed are committed. I.e., don't change the whitespace, don't touch lines not relevant to your changes, etc... Thanks.

@@ -7,3 +7,27 @@
<i class="glyphicon glyphicon-eye-open"></i>
_(Default: `false`)_ -
Whether the element should be collapsed or not.

Copy link
Contributor

Choose a reason for hiding this comment

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

Look at the uib-tab documentation for what to put in this block: default should be null, callback could also be an expression and not a function.

Copy link
Contributor

Choose a reason for hiding this comment

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

Check that, @wesleycho, what do you think about that last bit? Function vs. expression?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should read expression.

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 went off the uib-alert documentation which uses:

* `close`
  <small class="badge">$</small>
  _(Default: `none`)_ -
  A callback function that gets fired when an `alert` is closed. If the attribute exists, a close button is displayed as well.

But I'm happy to go with the format used by the uib-tab directive, if you prefer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, looks like uib-alert documentation is the exception. How does the following look?

* `collapsing()`
  <small class="badge">$</small> -
  An optional expression called before the element begins collapsing.
  If the expression returns a promise, animation won't start until the promise resolves.
  If the returned promise is rejected, collapsing will be cancelled.

* `collapsed()`
  <small class="badge">$</small> -
  An optional expression called after the element finished collapsing.

* `expanding()`
  <small class="badge">$</small> -
  An optional expression called before the element begins expanding.
  If the expression returns a promise, animation won't start until the promise resolves.
  If the returned promise is rejected, expanding will be cancelled.

* `expanded()`
  <small class="badge">$</small> -
  An optional expression called after the element finished expanding.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's be honest in here. Some callbacks has default and other doesn't. Leave it as is, I prefer this way.

@nonplus
Copy link
Contributor Author

nonplus commented Jan 13, 2016

All the changes are relevant. The indentation changes in collapse.js (collapse() and expand() functions) are due to the fact that the existing (unchanged) code is now inside of a $q.when() callback.

Not sure what else I ccould do about that. Or maybe I'm misunderstanding your request?

.addClass('collapsing')
.attr('aria-expanded', true)
.attr('aria-hidden', false);
$q.when(scope.$eval(attrs.expanding), function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be better to use $q.resolve.

In addition, perhaps it is better to do

$q.resolve(scope.$eval(attrs.expanding))
  .then(function() {
    ...
  });

since it is more in line with standard promise usage.

Copy link
Contributor

Choose a reason for hiding this comment

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

In addition, it would be better to cache the expression via var expandingExpr = $parse(attrs.expanding) earlier (and similarly for the other expressions), and then one can execute the expressions by doing expandingExpr(scope).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point about caching the expression.

FWIW, $q.resolve(x, callback) is more performant than $q.resolve(x).then(callback) since the latter waits for another tick. But I can make the change if you'd prefer.

@wesleycho
Copy link
Contributor

Thanks for the PR here!

This looks mostly solid - just would like to see those changes made with those minor optimizations and documentation updates, and should be good to get in afterwards.

@nonplus
Copy link
Contributor Author

nonplus commented Jan 13, 2016

Should be good to go.

@wesleycho
Copy link
Contributor

Can you squash this into one commit?

@wesleycho
Copy link
Contributor

This is good work - thanks for this PR!

@wesleycho wesleycho closed this in 446364a Jan 14, 2016
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.

4 participants