Add appendToBody and disableScroll props#1069
Conversation
|
You can try it with |
majapw
left a comment
majapw
left a comment
There was a problem hiding this comment.
This is really well written! This seems like a great addition to our API @joaovieira. Sorry for the delay in reviewing!
As for your comment about the anchors:
Note One issue is that the anchors are rendered with the input instead of the picker, so the z-index context is different. I feel this is odd and the anchors should be part of the picker/popup. Though this is a much bigger change. Let me know what you think.
I would say that would be part of a different change. I incorporated the anchor into the input because I wanted to positioning to be consistent with whichever DateInput was selected, regardless of how they were styled and this helped us accomplish that. We can def revisit that implementation.
My only comment is really to write a few more comments on the disableScroll utility because it's not immediately clear to me what is considered a scroll ancestor.
Other than that and some tests, this is awesome!
| @@ -0,0 +1,36 @@ | |||
| function getScrollParent(node, rootNode) { | |||
There was a problem hiding this comment.
Can you leave a comment as to what the scroll parent is? It seems like this will grab a modal body if that's the source of the picker, but I'm not entirely understanding how.
| }); | ||
|
|
||
| toggle(true); | ||
| return () => toggle(false); |
|
Great! Thanks for having a look @majapw. Will look into this later today:
|
Signed-off-by: João Vieira <joaoguerravieira@gmail.com>
|
@majapw I've added as much tests as I could. This a feature that is tied with the DOM, hence most of the tests require a browser and/or mounting (mocking all window/document would be too complex and/or tie the tests with the implementation even more). Thus, I've opened #1109 to fix Karma and ensure browser tests are mandatory in CI. Let me know your thoughts. |
majapw
left a comment
majapw
left a comment
There was a problem hiding this comment.
This is fantastic! :) I have one comment (and we can address it in a follow-up/I can make the change if necessary) but I think this is great! Thank you so much for the thorough commenting/test coverage!
| * - it is allowed to scroll via CSS ('overflow-y' not visible or hidden); | ||
| * - and its children/content are "bigger" than the node's box height. | ||
| * | ||
| * The root of the document always scrolls by default. |
There was a problem hiding this comment.
Thank you! This makes it super clear. :)
| } | ||
|
|
||
| return { | ||
| transform: `translateX(${offsetX}px) translateY(${offsetY}px)`, |
There was a problem hiding this comment.
Would it be better to use translate3D(${offsetX}px, ${offsetY}px, 0) here for performance reasons?
There was a problem hiding this comment.
You're right. Thought all CSS transforms were already hardware accelerated. I can do it. Thanks :)
|
Sweet, I'll merge this in after tests pass! |
3 participants
Fixes #273
This adds the ability to have the day picker render outside of the scrolling container instead of being clipped.
It uses the same Portal as the portal variations, but positions the picker next to the inputs via hardware-accelerated CSS
translate(keeping thetop/bottomfor relative positioning).On top of that, I've decided to disable the scroll in this situation to avoid the picker being displayed when the input goes "out-of-bounds"/overflows the parent container, in case we scroll the picker with the input (as Tether/Popper does).
I've also decided to add a
disableScrollwith the same behaviour for non-detached pickers.If you agree with this approach I'll continue to add tests.
Note One issue is that the anchors are rendered with the input instead of the picker, so the z-index context is different. I feel this is odd and the anchors should be part of the picker/popup. Though this is a much bigger change. Let me know what you think.
Before
After
Problem with a detached scrolling picker