Skip to content
This repository has been archived by the owner on Sep 5, 2024. It is now read-only.

fix(list): make secondary items static #7651

Closed

Conversation

devversion
Copy link
Member

  • 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)

@crisbeto Maybe you can also review

Fixes #7500. Fixes #2759.

@devversion devversion added the needs: review This PR is waiting on review from the team label Mar 20, 2016
}
// 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');
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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. 👍

@crisbeto
Copy link
Member

LGTM 👍

@devversion devversion force-pushed the wip/list-secondary-static branch from 31d626e to f1f4023 Compare March 20, 2016 17:00
* 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.
@devversion devversion force-pushed the wip/list-secondary-static branch from f1f4023 to c0ea4ae Compare March 22, 2016 21:19
@EladBezalel
Copy link
Member

LGTM, with that said, let this be one of the last PRs about list and focus on the new implementation,
@ThomasBurleson

@EladBezalel EladBezalel added pr: merge ready This PR is ready for a caretaker to review and removed needs: review This PR is waiting on review from the team labels Mar 22, 2016
@devversion
Copy link
Member Author

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:

  • The secondary items are now static (this was a bug since many months)
    I compared the new secondary items with the old items and focused on the specs.
    That means, padding etc is all the same.
  • The Proxy scan wasn't incorrect in the previous version. I've updated the scan logic here (no breaking change) and it works very well. (tested that with a lot user applications - codepens etc.)
  • I've improved the tests, to be more clear
    That means, now the tests are more accurate and easier to read.
  • Also RTL works very well on the list with the secondary items.

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 flexFiller, which moves the secondary items to the end of the list.

@devversion devversion added needs: work pr: merge ready This PR is ready for a caretaker to review and removed pr: merge ready This PR is ready for a caretaker to review needs: work labels Mar 22, 2016
gmoothart pushed a commit to gmoothart/material that referenced this pull request Apr 5, 2016
* 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
@devversion devversion deleted the wip/list-secondary-static branch April 19, 2016 19:48
@Splaktar Splaktar added this to the 1.1.0 milestone Sep 23, 2019
@angular angular locked as resolved and limited conversation to collaborators Sep 23, 2019
@Splaktar Splaktar added P4: minor Minor issues. May not be fixed without community contributions. type: bug labels Sep 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
P4: minor Minor issues. May not be fixed without community contributions. pr: merge ready This PR is ready for a caretaker to review type: bug
Projects
None yet
5 participants