-
Notifications
You must be signed in to change notification settings - Fork 3.4k
fix(list): make secondary items static #7651
Conversation
} | ||
// When using multiple secondary items we need to remove their secondary class to be | ||
// orderd correctly in the list-item | ||
secondaryItem.classList.remove('md-secondary'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not wrap this as a jqLite element and use removeClass
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no need, because, when iterating through a jqLite array, we retrieve always plain DOM elements.
So wrapping the element for one expression, is not needed in my opinion. In addition to that, it's just transferred this statement from the previous child block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, we should just bear in mind that classList
can be a little inconsistent in IE. The way you're using it here should be fine though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just rechecked that code snippet, because you mentioned me on the IE10/9 issues with classList
and figured out, that there's no need to remove that class anymore, because we are now always using a secondary container with static children. 👍
LGTM 👍 |
31d626e
to
f1f4023
Compare
* The secondary items always had an absolute position, which causes issues with overflowing. * This commit refactors the list to make the secondary items static. * Fixes multiple secondary padding (as in specs - consistent) * Fixes Proxy Scan for secondary items container * Improved tests for the list component (more clear and more safe) Fixes angular#7500. Fixes angular#2759.
f1f4023
to
c0ea4ae
Compare
LGTM, with that said, let this be one of the last PRs about list and focus on the new implementation, |
I already wrote with Elad about the implementation, and I'm looking forward with helping to plan the new implementation. Just want to summarize a few things here again:
So I can say, I tested these things thoroughly, which I've listed before. The only thing about I'am a bit unsure, is the |
* The secondary items always had an absolute position, which causes issues with overflowing. * This commit refactors the list to make the secondary items static. * Fixes multiple secondary padding (as in specs - consistent) * Fixes Proxy Scan for secondary items container * Improved tests for the list component (more clear and more safe) Fixes angular#7500. Fixes angular#2759. Closes angular#7651
@crisbeto Maybe you can also review
Fixes #7500. Fixes #2759.