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

Improve the fixed header approach to remove "header padding fouc" #1560

Closed
ang-zeyu opened this issue Apr 30, 2021 · 10 comments · Fixed by #1939
Closed

Improve the fixed header approach to remove "header padding fouc" #1560

ang-zeyu opened this issue Apr 30, 2021 · 10 comments · Fixed by #1939
Labels
Projects

Comments

@ang-zeyu
Copy link
Contributor

ang-zeyu commented Apr 30, 2021

Is your request related to a problem?

Browser refresh:
fixed-header-approach

Describe the solution you'd like

Experiment with a dynamic positioning approach, e.g. https://www.w3schools.com/howto/howto_js_sticky_header.asp.

Describe alternatives you've considered

  • Add a "best estimate" of the initial header height -- issue: some pages have headers, some don't. Also only an estimate, some fouc would still remain.

Additional context

#1534 eliminates fouc from vue rendering but fouc from the style detection of headers at runtime remains.

@ang-zeyu
Copy link
Contributor Author

Some existing solutions / compromises:

In general it seems like there is no "easy / ideal way out". The core of the problem is that style detection of the header height is required, something that isn't too common.

@ang-zeyu
Copy link
Contributor Author

I ended up sidetracking here and there and drafted 3 solutions below on top of the OP's, all of which achieve the aim of "no more fouc" but each has pros and cons.

  1. (OP's) Fix header padding fouc (1st version w/ dynamically fixed header) #1929
    • simplest option (implementation wise) that activates position: fixed only when the reader scrolls past the header. our fixed-header-padding class's height is also only "populated" once this is done.
    • pros: simple implementation
    • cons: not sticky (no element can be above the header) (e.g. announcement at the top of the page but should not follow the header as you scroll downward)
  2. (~70% complete, edge cases with window resizing remain) Fix header padding fouc (2nd version w/ sticky header, implemented w/ position:fixed) #1930
    • this is the most complex option. It implements a sticky header using position: fixed + scrolling detection.
    • pros: position: sticky positioning rules / nuances do not have to be followed (e.g. sticky element must be placed as a direct child of "scroll container"), increasing author convenience. Vs opt 3, parts of the sidebars will not be hidden too.
    • cons: very complex implementation wise (the pr is not quite done yet), not sure if its quite worth it. Not quite a canonical sticky positioning (per css spec) as well.
  3. Fix header padding fouc (3rd version w/ sticky header, implemented w/ position:sticky) #1931
    • uses position: sticky to implement the sticky header. the fixed/sticky-header-padding class is removed in this case
    • pros: simple implementation, author-friendly (no fixed/sticky-header-padding class)
    • cons: ❗ part of the sidebars will be hidden as you scroll down
  4. (not implemented) option 3 with dynamically hidden header. Will "resolve" option 3's con.

Honestly not too sure which we should go with.

  • Option 1 can be viable as a minimal addition on top of our existing implementation. Personally slightly tending toward this for now (then see how things turn out again later). But still worth discussing the rest.
  • Option 2 seems complex implementation wise, although it is the most "robust" (but dosen't quite conform to css "sticky" concepts / nuances which might need some elaborate explaining to authors).
  • Ideally option 3's con didn't exist, then it could work since we assume authors have some html knowledge (of position: sticky's nuances). Option 4 resolves it but dosen't look the best (the header is hidden).
    • the con seems ok for the site nav but particularly damaging to the page nav (e.g. here)

Wdyt? @jonahtanjz

ps: the tests in the PRs will be updated once this is resolved

@jonahtanjz
Copy link
Contributor

Option 1 can be viable as a minimal addition on top of our existing implementation. Personally slightly tending toward this for now (then see how things turn out again later). But still worth discussing the rest.

I am also leaning towards option 1 as it is the simplest implementation-wise compared to the other options.

cons: not sticky (no element can be above the header) (e.g. announcement at the top of the page but should not follow the header as you scroll downward)

Perhaps we can introduce a new component to MarkBind to support announcement bar? This seems like a good addition as currently announcements have to be implemented manually by the user. Having a native announcement bar in MarkBind should make it easier for users to add elements before/after the header and also toggle between fixed/non-fixed bar accordingly (#1631). This will also help to resolve most of the concerns if we go with Option 1.

@ang-zeyu
Copy link
Contributor Author

I am also leaning towards option 1 as it is the simplest implementation-wise compared to the other options.

👍, I'll cleanup the PR sometime soon and ping when its ready. (sorry, backtracking a little with the item you raised)

*Missed mentioning: while simple incrementally, option 3/4 should be the simplest when considered in whole.

Perhaps we can introduce a new component to MarkBind to support announcement bar? This seems like a good addition as currently announcements have to be implemented manually by the user. Having a native announcement bar in MarkBind should make it easier for users to add elements before/after the header and also toggle between fixed/non-fixed bar accordingly (#1631). This will also help to resolve most of the concerns if we go with Option 1.

Thanks for raising this.. hmm...

I think we should we leave the announcement bar design to the author, since its one of the reasons of having expressive layouts. Also quite a few announcement bar designs to account for in a single component. More importantly it shouldn't be too much trouble to design this one-off thing from the author side (maybe @damithc can chip in here w.r.t to that discussion). But I'm ok if this can add enough value otherwise. (e.g. navbars are also one off but feature rich enough to warrant them)

But more importantly (not sure if this was accounted for) supporting this would ultimately still require implementing sticky positioning (via option 2 or 3) as well.


I think that discussion thread also brings up again whether supporting something like a fixed-header-padding is a flexible enough (in addition to not being user friendly) in the first place. (ref #1631 (reply in thread))

Previously I imagined you could place the banner below the header as a workaround (but didn't recall that discussion).

Wonder if we should go with 3/4 after all as such, it seems to be more generalisable although not optimal-looking. (at best the header is hidden when you scroll down)


Likely problematic food for thought:

To implement a fixed-positioned header, maybe instead we provide some instructions in the documentation on how to do so, following the most common solution around:

position: fixed + we know the height of the header beforehand (e.g. see https://vuepress.vuejs.org/guide/): unfortunately not an option in our case (by far the most common one around)

i.e. author manually measures the header size on some screen sizes and sets the padding-top manually. Although, even this is sometimes tricky (differing user agent stylesheets for one can change font styles/sizes/paddings and thus the height of headers).


or we can support both options 1 & 4, and advise when to use each in the documentation.

maybe one thing we can agree on first is that option 2 is definitely not the way to go (interesting experiment but difficult to explain and too complex (not even done yet) in implementation 😅)

@tlylt
Copy link
Contributor

tlylt commented May 17, 2022

I think we should we leave the announcement bar design to the author, since its one of the reasons of having expressive layouts. Also quite a few announcement bar designs to account for in a single component. More importantly it shouldn't be too much trouble to design this one-off thing from the author side (maybe @damithc can chip in here w.r.t to that discussion). But I'm ok if this can add enough value otherwise. (e.g. navbars are also one off but feature rich enough to warrant them)

Personally, I think it would be a good addition to have an announcement bar component (or minimally give an example of how to achieve that in our UG). I think the one that's on the 2103 website is an excellent example of such a use case and I feel that this can be generalized to other situations as well for our potential users. Perhaps going further, we may also consider supporting a "toast" component in the future.

(OP's) #1929
simplest option (implementation wise) that activates position: fixed only when the reader scrolls past the header. our fixed-header-padding class's height is also only "populated" once this is done.
pros: simple implementation
cons: not sticky (no element can be above the header) (e.g. announcement at the top of the page but should not follow the header as you scroll downward)

Just to chip in, I feel that if we can mitigate the cons here then this solution looks pretty good. Actually, I am not sure what you mean by "announcement at the top of the page but should not follow the header as you scroll downward)", so initially the announcement will be at the top, and if you scroll down, it follows? (In that case, perhaps if the announcement is dismissible, then this would still be somewhat ok?)...just some thoughts for discussion :)

@damithc
Copy link
Contributor

damithc commented May 17, 2022

I think we should we leave the announcement bar design to the author, since its one of the reasons of having expressive layouts. Also quite a few announcement bar designs to account for in a single component. More importantly it shouldn't be too much trouble to design this one-off thing from the author side (maybe @damithc can chip in here w.r.t to that discussion). But I'm ok if this can add enough value otherwise. (e.g. navbars are also one off but feature rich enough to warrant them)

Personally, I think it would be a good addition to have an announcement bar component (or minimally give an example of how to achieve that in our UG). I think the one that's on the 2103 website is an excellent example of such a use case and I feel that this can be generalized to other situations as well for our potential users. Perhaps going further, we may also consider supporting a "toast" component in the future.

Flexibility for authors is good. If we can leave room for that, we should. And yes, that should be accompanied by good examples in the UG.
Having a built-in component that can cover basic use cases (similar to the one in CS2103 website) can add value nevertheless, if not too much effort and does not reduce author flexibility. One way we want MarkBind to stand out from the rest is by providing everything the target user needs 'out of the box, batteries included'.

@ang-zeyu
Copy link
Contributor Author

Personally, I think it would be a good addition to have an announcement bar component (or minimally give an example of how to achieve that in our UG). I think the one that's on the 2103 website is an excellent example of such a use case and I feel that this can be generalized to other situations as well for our potential users. Perhaps going further, we may also consider supporting a "toast" component in the future.

Flexibility for authors is good. If we can leave room for that, we should. And yes, that should be accompanied by good examples in the UG.

Agreed, some instructions would be nice 😬

As for the component, below code example is taken from the 2103 site; I think it's really easy to construct on the author side, and may be counterproductive for us to limit the design using a component (at least, I can't imagine how we can provide flexibility in such a component 😅).

<div class="w-100 p-1 bg-warning text-center">

**This site is from a past semester! The current version will be [here](http://www.comp.nus.edu.sg/~cs2103") when the new semester starts.**
</div>

Toasts sound good! I think we can open up a separate issue for it.

Just to chip in, I feel that if we can mitigate the cons here then this solution looks pretty good. Actually, I am not sure what you mean by "announcement at the top of the page but should not follow the header as you scroll downward)", so initially the announcement will be at the top, and if you scroll down, it follows? (In that case, perhaps if the announcement is dismissible, then this would still be somewhat ok?)...just some thoughts for discussion :)

Pretty much what's described here #1631.
(but imagine the announcement being on top of the navbar instead)

Dismissible sounds good but ideally the reader shouldn't have to act on this at all :)

mitigate the cons

If you mean sticky positioning (but without the cons of that too) that would be solution 2, but its a little too complex for comfort. (see https://github.com/MarkBind/markbind/pull/1930/files#diff-6ae0c610a951c1b0540c673c0f37f26de23750e00c3c85c6e92ce5ddad97b89bR52)

I imagine there also are many edge cases (ref the scroll handling) not withstanding the window resizing ones I haven't accounted for. Might be good to stick to something simpler / predictable for an option (<header fixed/sticky>) that should be generalisable (in the sense of not too many restrictions + things the author has to take note of when using it).

@jonahtanjz
Copy link
Contributor

jonahtanjz commented May 18, 2022

Wonder if we should go with 3/4 after all as such, it seems to be more generalisable although not optimal-looking. (at best the header is hidden when you scroll down)

I think as mentioned earlier in your comment, option 3 is quite damaging to the page nav (the header + first item get hidden behind). I personally prefer option 1 over option 3 (option 4 may be a better alternative).

or we can support both options 1 & 4, and advise when to use each in the documentation.

I think this can be a good compromise if we allow user to choose one.

But more importantly (not sure if this was accounted for) supporting this would ultimately still require implementing sticky positioning (via option 2 or 3) as well.

For option 1, maybe instead of an announcement bar component, we can include in the documentation to give it a certain id? Then we can include this element in the calculation of the fixed header padding as well.

Something like this:

<div id="header_banner_top">...</div>
<header fixed>
...
</header>
<div id="header_banner_bottom">...</div>

This gives users flexibility to customize their own banners as well as allows for non-fixed banners with fixed header.

@ang-zeyu
Copy link
Contributor Author

I think this can be a good compromise if we allow user to choose one.

👍, I'll go with this then, then try to get @damithc in the PR to see from a author perspective if it checks out.

cons: not sticky (no element can be above the header) (e.g. announcement at the top of the page but should not follow the header as you scroll downward)

Perhaps we can introduce a new component to MarkBind to support announcement bar? This seems like a good addition as currently announcements have to be implemented manually by the user. Having a native announcement bar in MarkBind should make it easier for users to add elements before/after the header and also toggle between fixed/non-fixed bar accordingly (#1631). This will also help to resolve most of the concerns if we go with Option 1.

For option 1, maybe instead of an announcement bar component, we can include in the documentation to give it a certain id? Then we can include this element in the calculation of the fixed header padding as well.

(for header_banner_top) This won't work unfortunately, we'll need to offset the top: 0 of the <header fixed> in that case to preserve the stickiness of the header only, since it is implemented using position: fixed (for the announcement bar to not scroll downward with it) . (i.e. it reduces to something like option 2)

(for header_banner_bottom) do you mean something like this?

<header fixed>
...
</header>
<div id="header_banner_bottom" class="fixed-header-padding">...</div>
... site nav page nav content etc. *without* fixed header padding class ...

the same issue of the page nav / sitenav being hidden appears in this case too unfortunately, since they are no longer affected by fixed-header-padding.
Adding fixed-header-padding causes a "double padding". (huge empty space between the announcement and site/page/content once we populate fixed-header-padding)

@jonahtanjz
Copy link
Contributor

(for header_banner_top) This won't work unfortunately, we'll need to offset the top: 0 of the

in that case to preserve the stickiness of the header only, since it is implemented using position: fixed (for the announcement bar to not scroll downward with it) . (i.e. it reduces to something like option 2)

(for header_banner_bottom) do you mean something like this?

<header fixed>
...
</header>
<div id="header_banner_bottom" class="fixed-header-padding">...</div>
... site nav page nav content etc. *without* fixed header padding class ...

the same issue of the page nav / sitenav being hidden appears in this case too unfortunately, since they are no longer affected by fixed-header-padding.
Adding fixed-header-padding causes a "double padding". (huge empty space between the announcement and site/page/content once we populate fixed-header-padding)

Ahh ok I see the issue.

👍, I'll go with this then, then try to get @damithc in the PR to see from a author perspective if it checks out.

Yup let's go with this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Big Picture
Completed
Development

Successfully merging a pull request may close this issue.

4 participants