-
Notifications
You must be signed in to change notification settings - Fork 14
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
Add spacer element for non-sticky situations #7
base: master
Are you sure you want to change the base?
Conversation
@ykdr2017 Thank you so much for the thoughtful description, the codepen and the PR! Let's a get a similar feature in for sure. Question: Can |
@yowainwright Thank you for quickly replying. I have considered to add padding once, but I think padding may be not useful in certain cases. Especially if
|
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.
Hey, thanks @ykdr2017 for your thoughtful PR and comments. I wrote some quick notes. Lets see where we get from there. 🙏
@@ -164,13 +164,17 @@ class Stickybits { | |||
- offset = number | |||
- stickyStart = number | |||
- stickyStop = number | |||
- spacer = dom element | null |
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.
This should be controlled by the plugin.
- It should go right into
position: fixed
code (not optional). - It should provide a css class
stickybits-item-stub
, or something like that. - The inline css of the stub should have a height set to the calculated stickybit item height.
- The stub should be set to
display:block;
vsdisplay:none
when the stickybit item is set toposition: fixed
(but I know it is a bit more nuanced than that 😄).
See Fixed Sticky for reference.
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.
@yowainwright Thank you for comments 👍 Let me discuss before fixing.
- It should go right into position: fixed code (not optional).
I agree that in terms of minimizing responsibility of instance. However I feel a bit uncertain that stickybits has to get stub element reference from current DOM tree again when destroying instance.
fixed-sticky does so , and it can lose the reference when re-getting. ( I know it's very rare case, but possible because DOM tree should be regarded as a global variable)
- It should provide a css class
stickybits-item-stub
, or something like that.
OK, I'll add it after the other discussion!
- The inline css of the stub should have a height set to the calculated stickybit item height.
- The stub should be set to
display:block;
vsdisplay:none
when the stickybit item is set toposition: fixed
(but I know it is a bit more nuanced than that 😄).
They seem to be implemented around here, could you tell me if it is different from you intended ?
Lines 381 to 390 in cdc8a85
spacerStyles: { | |
...(pv === 'fixed' ? { | |
height: `${spacerHeight}px`, | |
display: spacerDisplay, | |
} : { | |
// Omit spacer if position is not 'fixed', | |
// even if position of state stuck is 'absolute' | |
display: 'none', | |
}), | |
}, |
Note: spacerDisplay
is assumed it can be block
or something else, such as table-header-group
.
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.
Sorry @ykdr2017, I was just writing out instructions. 😃
As this issue supports a very small (but important use-case), I want to ensure that the footprint of this update is small.
@ykdr2017 I'm going to pull your PR and make a few tweaks within the next few days. Thanks again! |
Thank you very much for amazing library!
Fixes
useFixed: true
or IE11.position: sticky
is replaced with other styles,fixed
andabsolute
. However,position: sticky
keeps gap where the element was originally located and the others don't.fixed
andabsolute
shorten the element's parent height, and derails appearance and the logic of changing state (default
,sticky
,stuck
in the implementation ).reproduction: https://codepen.io/ykdr2017/pen/xxwGEbE
Proposed Changes
spacer
for non-sticky situations. The spacer has the same height as the target element, and the parent will keep its height.