Skip to content

fix(core): check for undefined on sort priority #5000

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jan 29, 2016
Merged

fix(core): check for undefined on sort priority #5000

merged 2 commits into from
Jan 29, 2016

Conversation

dlgski
Copy link
Contributor

@dlgski dlgski commented Jan 20, 2016

in Issue #4876 isSortPriorityVisible() is introduced. However it is testing the variable rather than for undefined, so when element.sort.priority == 0, it is returning as false, even though that should return true.

This update is adding the checks for undefined

@jbarrus @JLLeitschuh

@dlgski
Copy link
Contributor Author

dlgski commented Jan 20, 2016

A better fix for this would be to index sort.priority at 1 rather than 0, but I cannot find where sort.priority is getting initially set to 0.

If sort.priority was indexed at 1, then this code would have worked the way it was. Also, having an indicator of "0" looks kind of odd. (which this will result with)

@swalters
Copy link
Contributor

Would this PR fix the issue? #4807

@dlgski
Copy link
Contributor Author

dlgski commented Jan 22, 2016

I added some comments to that PR. If that PR is indexing sortPriority at
1, then I think these checks will properly return "true" when sortPriority
exists

On Thu, Jan 21, 2016 at 5:16 PM, Shane Walters notifications@github.com
wrote:

Would this PR fix the issue? #4807
#4807


Reply to this email directly or view it on GitHub
#5000 (comment).

@JLLeitschuh
Copy link
Contributor

This makes sense since 0 would evaluate to false in this case. Okay, cool Thanks.

@swalters swalters changed the title check for undefined on sort priority fix(core): check for undefined on sort priority Jan 29, 2016
swalters added a commit that referenced this pull request Jan 29, 2016
fix(core): check for undefined on sort priority
@swalters swalters merged commit 29eb6e1 into angular-ui:master Jan 29, 2016
@dlgski dlgski deleted the sortpriority branch December 6, 2016 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants