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

fix(dropdown): use $animate for adding and removing classes #1644

Closed
wants to merge 2 commits into from
Closed

fix(dropdown): use $animate for adding and removing classes #1644

wants to merge 2 commits into from

Conversation

olostan
Copy link
Contributor

@olostan olostan commented Jan 22, 2014

As AngularJS 1.2 was released, there is a need to use $animate for adding and removing classes.

@olostan olostan mentioned this pull request Jan 22, 2014
Closed
@mvhecke
Copy link
Contributor

mvhecke commented Jan 22, 2014

This pull request fails to build because your if statement is incorrect.

@pkozlowski-opensource
Copy link
Member

@olostan this PR introduces dependency on $animate and as such would require way more work:

  • update to documentation
  • update to tests
  • update to the build system

All those things are missing atm....

@olostan
Copy link
Contributor Author

olostan commented Jan 23, 2014

@Gamemaniak Sorry, was too busy to run tests. Now fixed: "Chrome 31.0 (Windows): Executed 549 of 549 SUCCESS (9.025 secs / 8.47 secs) "

@pkozlowski-opensource Actually $animate in included into base Angular 1.2+ distribution, so no additional dependency is needed. It works in such way:

  1. If you add dependency to ngAnimate, animation module do all animation magic with classes
  2. BUT if you do not include any dependency in your application, default implementation to addClass/removeClass is used.

So this change does not introduce any additional dependency, so no need to modify documentation/test/build system.

@pkozlowski-opensource
Copy link
Member

@olostan oh, sorry. @chrisirhc do you want to look into this one as part of $animate?

@ghost ghost assigned chrisirhc Jan 23, 2014
@chrisirhc
Copy link
Contributor

Yes, I'll look into this. I want to make sure this can't be done with ngClass first.

chrisirhc
chrisirhc reviewed Jan 25, 2014
View reviewed changes
src/dropdown/dropdown.js
if (value) {
$animate.addClass(self.$element, openClass);
}
else {
Copy link
Contributor

Choose a reason for hiding this comment

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

The else should be in the same line as the closing braces of the if (} else {).

@chrisirhc
Copy link
Contributor

Just got to this. Looks good to me. 👍
This PR doesn't need other changes (besides styling) as long as we've already added the AngularJS 1.2 requirement in the readme. (which we already have)

bekos
bekos reviewed Jan 29, 2014
View reviewed changes
src/dropdown/dropdown.js
@@ -54,7 +54,12 @@ angular.module('ui.bootstrap.dropdown', [])
};

$scope.$watch('isOpen', function( value ) {
self.$element.toggleClass( openClass, value );
if (value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

There is already an if clause for value. No need to add a new one.

Copy link
Contributor

Choose a reason for hiding this comment

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

These lines can actually be shortened to:

$animate[value ? 'addClass' : 'removeClass'](self.$element, openClass);

Copy link
Contributor

Choose a reason for hiding this comment

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

$animate should have a promise, and the code after that should be inside the resolved function. Else the toggle function would be called before the dropdown is opened.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This pull request is closed as I see (it was created looong time ago).

@pkozlowski-opensource
Copy link
Member

@bekos @chrisirhc could the one be landed?

@bekos bekos closed this in e8d5fef Feb 8, 2014
benjaminch pushed a commit to benjaminch/bootstrap that referenced this pull request Apr 15, 2014
@bekos @benjaminch
fix(dropdown): unbind toggle element event on scope destroy
6467826 
Also, change the way disabled dropdownToggle is read from `attrs` instead of element property.

Closes angular-ui#1867
Closes angular-ui#1870

Add an optionnal parameter on timepicker object in order to control the visibility of the controls arrows.

chore(tests): Add unit tests for arrows control visibility

Fixing demo semi colon issue.

chore(release): Add unit tests

chore(timepicker): Add LOC to check the user's input

chore(timepicker): Remove  method for showArrowControls option

chore(timepicker): Remove Show / Hide option from demo

chore(timepicker): Hides <tr> element rather than <td> element.

chore(tests): Simplify arrows control visibility tests

chore(tests): Refactoring arrow visibility tests

refactor(rating): use `track by` in template

Closes angular-ui#1724

refactor(pagination): add `href` and `track by` in templates

Closes angular-ui#1725

fix(progressbar): allow fractional values for bar width

 * Fractions are limited to two decimals, and are not included in aria values.

Closes angular-ui#1761

fix(dropdown): use $animate for adding and removing classes

Closes angular-ui#1644

feat(datepicker): add `datepicker-mode`, `init-date` & today hint

 * Add two-way binded `datepicker-mode`.
 * Add optional `init-date` when no model value is specified.
 * Add hint for current date.
 * Use isolated scope for popup directive.
 * Use optional binding for `isOpen`.
 * Split each mode into it's own directive.

Closes angular-ui#1599

BREAKING CHANGE:
`show-weeks` is no longer a watched attribute
`*-format` attributes have been renamed to `format-*`
`min` attribute has been renamed to `min-date`
`max` attribute has been renamed to `max-date`

refactor(rating): evaluate attributes inside init function

Closes angular-ui#1590
Closes angular-ui#1768

refactor(pagination): remove unused watch

Closes angular-ui#1769

fix(typeahead): correctly higlight numeric matches

Fixes angular-ui#1777

refactor(typeahead): use `ng-if` and `track by` in template

Closes angular-ui#1722

demo(typeahead): correct demo for asyn results

Fixes angular-ui#1740

refactor(datepicker): track buttons by date instead of $index

refactor(pagination): move boundary & directions to the template

Closes angular-ui#795
Closes angular-ui#1770

feat(alert): add WAI-ARIA markup

Closes angular-ui#1806

chore(demo): fix fork button width

Closes angular-ui#1805

fix(datepicker): `Today` button should not set time

Fixes angular-ui#1726
Closes angular-ui#1808

fix(progressbar): number filter in bar template and only for percent

Closes angular-ui#1807

refactor(dropdown): remove isolated scope

Closes angular-ui#1818

feat(typeahead): add WAI-ARIA markup

Closes angular-ui#1814

refactor(accordion): transclude function in compile is deprecated.

Closes angular-ui#1789

refactor(carousel): use `track by` in template

Closes angular-ui#1723

fix(datepicker): mark input field as invalid if the date is invalid

Closes angular-ui#1845

Conflicts:
	src/datepicker/datepicker.js
	src/datepicker/test/datepicker.spec.js
benjaminch pushed a commit to benjaminch/bootstrap that referenced this pull request May 10, 2014
@bekos @benjaminch
fix(dropdown): unbind toggle element event on scope destroy
36dd04f 
Also, change the way disabled dropdownToggle is read from `attrs` instead of element property.

Closes angular-ui#1867
Closes angular-ui#1870

Add an optionnal parameter on timepicker object in order to control the visibility of the controls arrows.

chore(tests): Add unit tests for arrows control visibility

Fixing demo semi colon issue.

chore(release): Add unit tests

chore(timepicker): Add LOC to check the user's input

chore(timepicker): Remove  method for showArrowControls option

chore(timepicker): Remove Show / Hide option from demo

chore(timepicker): Hides <tr> element rather than <td> element.

chore(tests): Simplify arrows control visibility tests

chore(tests): Refactoring arrow visibility tests

fix(dropdown): unbind toggle element event on scope destroy

Also, change the way disabled dropdownToggle is read from `attrs` instead of element property.

Closes angular-ui#1867
Closes angular-ui#1870

Add an optionnal parameter on timepicker object in order to control the visibility of the controls arrows.

chore(tests): Add unit tests for arrows control visibility

Fixing demo semi colon issue.

chore(release): Add unit tests

chore(timepicker): Add LOC to check the user's input

chore(timepicker): Remove  method for showArrowControls option

chore(timepicker): Remove Show / Hide option from demo

chore(timepicker): Hides <tr> element rather than <td> element.

chore(tests): Simplify arrows control visibility tests

chore(tests): Refactoring arrow visibility tests

refactor(rating): use `track by` in template

Closes angular-ui#1724

refactor(pagination): add `href` and `track by` in templates

Closes angular-ui#1725

fix(progressbar): allow fractional values for bar width

 * Fractions are limited to two decimals, and are not included in aria values.

Closes angular-ui#1761

fix(dropdown): use $animate for adding and removing classes

Closes angular-ui#1644

feat(datepicker): add `datepicker-mode`, `init-date` & today hint

 * Add two-way binded `datepicker-mode`.
 * Add optional `init-date` when no model value is specified.
 * Add hint for current date.
 * Use isolated scope for popup directive.
 * Use optional binding for `isOpen`.
 * Split each mode into it's own directive.

Closes angular-ui#1599

BREAKING CHANGE:
`show-weeks` is no longer a watched attribute
`*-format` attributes have been renamed to `format-*`
`min` attribute has been renamed to `min-date`
`max` attribute has been renamed to `max-date`

refactor(rating): evaluate attributes inside init function

Closes angular-ui#1590
Closes angular-ui#1768

refactor(pagination): remove unused watch

Closes angular-ui#1769

fix(typeahead): correctly higlight numeric matches

Fixes angular-ui#1777

refactor(typeahead): use `ng-if` and `track by` in template

Closes angular-ui#1722

demo(typeahead): correct demo for asyn results

Fixes angular-ui#1740

refactor(datepicker): track buttons by date instead of $index

refactor(pagination): move boundary & directions to the template

Closes angular-ui#795
Closes angular-ui#1770

feat(alert): add WAI-ARIA markup

Closes angular-ui#1806

chore(demo): fix fork button width

Closes angular-ui#1805

fix(datepicker): `Today` button should not set time

Fixes angular-ui#1726
Closes angular-ui#1808

fix(progressbar): number filter in bar template and only for percent

Closes angular-ui#1807

refactor(dropdown): remove isolated scope

Closes angular-ui#1818

feat(typeahead): add WAI-ARIA markup

Closes angular-ui#1814

refactor(accordion): transclude function in compile is deprecated.

Closes angular-ui#1789

refactor(carousel): use `track by` in template

Closes angular-ui#1723

fix(datepicker): mark input field as invalid if the date is invalid

Closes angular-ui#1845

Conflicts:
	src/datepicker/datepicker.js
	src/datepicker/test/datepicker.spec.js
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@chrisirhc chrisirhc

Labels
component: dropdown type: ngAnimate
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@olostan @mvhecke @pkozlowski-opensource @chrisirhc @bekos @jainsneha23 @karianna