Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ykdr8
Copy link

@ykdr8 ykdr8 commented Apr 13, 2020

Thank you very much for amazing library!

Fixes

  • This PR fixes a problem when using configuration useFixed: true or IE11.
  • In these situations, position: sticky is replaced with other styles, fixed and absolute. However, position: sticky keeps gap where the element was originally located and the others don't. fixed and absolute 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

  • Inserted an element called spacer for non-sticky situations. The spacer has the same height as the target element, and the parent will keep its height.
  • This approach may be just one of many ways. Any alternatives or problems are welcome.

@ykdr8 ykdr8 requested a review from yowainwright as a code owner April 13, 2020 00:53
@yowainwright
Copy link
Owner

yowainwright commented Apr 13, 2020

@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 padding-top be added to the patent to match the sticky element's height?

@ykdr8
Copy link
Author

ykdr8 commented Apr 13, 2020

@yowainwright Thank you for quickly replying.

I have considered to add padding once, but I think padding may be not useful in certain cases.
One of them is the case there is another content just above stickybit. The gap should be created between stickybit and content below, but it seems to be hard to do by the approach of adding padding to the parent.

Especially if stickyBitStickyOffset is set, it could be visible that upper content and the content under stickybit are glued. It seems to be hard to resolve by parent padding.

stickyBitStickyOffset example: https://codepen.io/ykdr2017/pen/qBOdPXQ

Copy link
Owner

@yowainwright yowainwright left a 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
Copy link
Owner

@yowainwright yowainwright Apr 13, 2020

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; vs display:none when the stickybit item is set to position: fixed (but I know it is a bit more nuanced than that 😄).

See Fixed Sticky for reference.

Copy link
Author

@ykdr8 ykdr8 Apr 14, 2020

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; vs display:none when the stickybit item is set to position: 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 ?

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 .

Copy link
Owner

@yowainwright yowainwright Apr 14, 2020

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.

@yowainwright
Copy link
Owner

@ykdr2017 I'm going to pull your PR and make a few tweaks within the next few days.

Thanks again!

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