Skip to content

Comments

Add support for container padding#57

Merged
tbranyen merged 1 commit intotbranyen:masterfrom
stevenvachon:master
Jul 3, 2019
Merged

Add support for container padding#57
tbranyen merged 1 commit intotbranyen:masterfrom
stevenvachon:master

Conversation

@stevenvachon
Copy link
Contributor

  • Padding is computed, which supports values with em and other units as they are automatically converted to px
  • Top/Bottom or left/right padding is added to the sizer ("scroller")
  • Top/Left padding (which precedes the first item) is added to each item's style position, but not the scroll position, thereby retaining the padding values when using scrollTo with _itemPositions. For example, scrolling to item 0 would have otherwise never reached the the beginning (0px).
  • Some tests were updated because now the scroller has a top CSS value.

applyPatch (element, fragment) {
childNodes = fragment.map(childNode => {
if (!childNode.style.top) {
if (!childNode.style.top || childNode.nodeName === 'tr') {
Copy link
Owner

Choose a reason for hiding this comment

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

What was this change about?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously, the sizer element (<tr>) didn't have a top css property, which met the test's condition for exclusion. Now that it does have that property, the condition needed to be updated.

Copy link
Owner

Choose a reason for hiding this comment

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

<tr> can be changed via scrollerTagName so we should probably use that instead of hardcoding tr.

@tbranyen
Copy link
Owner

Very cool, looks great to me. I like how it only gets calculated during refresh. I'll review it in more depth and if anything sticks out will comment, otherwise we can merge.

@stevenvachon
Copy link
Contributor Author

@tbranyen Any update? It'd be best if this didn't go stale.

@tbranyen
Copy link
Owner

@stevenvachon only had that one last comment about the tr and once that is resolved, I think we're good.

@stevenvachon
Copy link
Contributor Author

@tbranyen scrollerTagName doesn't have a default value in the config, though; only in refresh().

@tbranyen
Copy link
Owner

tbranyen commented Jul 3, 2019

@stevenvachon I think from a code perspective it looks good, but we need to add some documentation around the change in the README or even an example.

@stevenvachon
Copy link
Contributor Author

I don't think this PR warrants a documentation change. It now simply behaves as you'd expect it to if the scrolling container were to have padding.

@tbranyen tbranyen merged commit dda9904 into tbranyen:master Jul 3, 2019
@tbranyen
Copy link
Owner

tbranyen commented Jul 3, 2019

Published in 1.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants