Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

fix(select): Fix several issues when moving options between groups - #10166

Closed
lgalfaso wants to merge 1 commit into
angular:masterfrom
lgalfaso:select-fixes
Closed

lgalfaso wants to merge 1 commit into
angular:masterfrom
lgalfaso:select-fixes

Conversation

@lgalfaso

Copy link
Copy Markdown
Contributor
  • When an option was moved to a previous group, the group that loose the option would remove the label from the controller
  • When an entire option group was removed, the options in the group were mot removed from the controller

@googlebot

Copy link
Copy Markdown

CLAs look good, thanks!

Comment thread test/ng/directive/selectSpec.js Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

two empty blanks betweets its please. one day we'll have a jscs rule for this :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this test is based on #10161 (at 1.3 this is working, but I will need to backport this to 1.2). Anyhow, will add the extra checks

@lgalfaso

Copy link
Copy Markdown
Contributor Author

updated the PR with the proposed changes

@pkozlowski-opensource

Copy link
Copy Markdown
Member

@lgalfaso I've added it to the 1.3.5 milestone. Feel free to move it to 1.3.4 if you think it is close to being ready to merge. I'm restarting Travis to see what it thinks about your PR :-) (BTW, Travis is reallly flaky today...)

@lgalfaso

Copy link
Copy Markdown
Contributor Author

@pkozlowski-opensource if I get a 👍 from a core member team, I would like to commit this for 1.3.4 (and backport it so it makes it to 1.2.28)

Comment thread test/ng/directive/selectSpec.js Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

"an element" or "elements"?

@Narretz

Narretz commented Nov 26, 2014

Copy link
Copy Markdown
Contributor

I reviewed it too because I work on another select bug, and it's good practive in understanding this beast ;)
LGTM, only two little nitpicks

* When an option was moved to a previous group, the group that
loose the option would remove the label from the controller
* When an entire option group was removed, the options in the
group were mot removed from the controller
@lgalfaso

Copy link
Copy Markdown
Contributor Author

Updated the PR

@petebacondarwin

Copy link
Copy Markdown
Contributor

LGTM - I'll fix up that one element.controller bit and merge
Thanks @lgalfaso

@lgalfaso lgalfaso closed this in 30694c8 Nov 27, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants