Fix heading padding FOUC (option 3 with modifications: sticky header) #1939

ang-zeyu · 2022-05-25T08:24:00Z

What is the purpose of this pull request?

Resolves #1560

Follow up on #1929, #1930, #1931 and discussion in #1560

Overview of changes:

Adds this feature: https://deploy-preview-1939--markbind-master.netlify.app/userguide/tweakingthepagestructure#option-2-sticky-header-dynamically-hidden
Changes this https://deploy-preview-1939--markbind-master.netlify.app/userguide/tweakingthepagestructure#option-1-fixed-header to be dynamically hidden (see yellow warning box)

Anything you'd like to highlight / discuss:

Is the feature set o-k? (too complex? lacking in configuration?)
This is somewhat breaking change as it requires making some small adjustments in the default user layout files (move initial padding-top to margin). ❗ Should be noted in the release changelog as it requires user modification.

Testing instructions:

<header fixed> (anywhere in the userguide except userGuide/stickyDemo.html) should behave as before, except without the FOUC. This includes the mobile site / page nav.
- https://deploy-preview-1939--markbind-master.netlify.app/userguide/tweakingthepagestructure
<header sticky> (new feature) performs as described in the documentation. The mobile site / page nav should perform identically to that of <header fixed> (currently broken - tbd done!)
- https://deploy-preview-1939--markbind-master.netlify.app/userguide/stickydemo

Proposed commit message: (wrap lines at 72 characters)
Implement sticky header

Since no assumptions of the user's header's height can be made at build
time, runtime style detection is required to calculate the header
height used in the fixed-header-padding class. This causes a flash of
unstyled content briefly before this detection is done
(the content is hidden behind the header).

Let's implement a sticky header instead, which "natively" offsets
content below it.

It should be noted that header height detection is still required
however, particularly for offsetting the sticky positioning of the
navigation menus on the side (top: var(--sticky-header-height)), since
they are (usually) much shorter than the main content element.

This header height is now exposed using the css variable
var(--sticky-header-height) for use in the user layout files.

Checklist: ☑️

Updated the documentation for feature additions and enhancements
Added tests for bug fixes or features (we need e2e tests)
Linked all related issues
No unrelated changes

ang-zeyu · 2022-05-25T08:35:51Z

Any thoughts on the conclusion @damithc?

Does this make sense to a MarkBind user? (complexity / insufficient configuration / etc.) 👀

https://deploy-preview-1939--markbind-master.netlify.app/userguide/tweakingthepagestructure#sticking-the-header-to-the-top

damithc · 2022-05-25T09:56:58Z

Any thoughts on the conclusion @damithc?

Does this make sense to a MarkBind user? (complexity / insufficient configuration / etc.) 👀

While a bit more complex than usual, I think it is still acceptable if we fine-tune the description to be bit more noob-friendly.

jonahtanjz · 2022-05-28T16:52:37Z

packages/core-web/src/index.js

  window.addEventListener('resize', () => {
-    if (window.innerWidth > 767 && headerSelector.hasClass('hide-header')) {
+    if (window.innerWidth > 767 && isHidden) {


Just a heads up, I think will have to update the fixed-header-padding height one more time during resize.

Currently there will be a large padding if the header reappears during resize:

MarkBind.-.User.Guide_.Presentational.Components.-.Google.Chrome.2022-05-29.00-48-02.mp4

tlylt

Added some comments on the UG :)

docs/_markbind/layouts/userGuide.md

docs/userGuide/tweakingThePageStructure.md

ang-zeyu · 2022-06-10T08:59:14Z

@jonahtanjz @tlylt @damithc Thanks for the reviews!

After letting this sit for a while I realised there was a very simple way I missed previously to achieve the best result:

in terms of reader functionality, this results in the benefits of the previously discussed option 2. No fouc, allow non-sticky announcements / banners, and site / page nav is not hidden behind the header.
resulting implementation is also no where as complex as option 2

The gist:

I've removed option 1 (dynamically fixed header). This works, but again the drawback is that non-sticky announcements cannot be placed.
I've adapted option 3. The easy fix here is to offset the sticky positioning of the site / page nav in the layout files by the sticky header height. This has all the benefits of option 2 (no fouc, allows things on top of the header, ...)
- I've also exposed the --sticky-header-height css variable in the user guide for advanced usage. This can be seen as the same slightly awkward fixed-header-padding class, so its still slightly complex in terms of author usability however.
In terms of documentation best reviewed from scratch. 😓 There are a lot of test file updates (from fixed-header-padding removal mainly) so github's file explorer should really help

Concerns:

Mainly whether the user guide is clear enough due to exposing the css variable.
https://deploy-preview-1939--markbind-master.netlify.app/userguide/tweakingthepagestructure#sticking-the-header-to-the-top

ang-zeyu

some self-commenting on the changes to hopefully make reviewing easier :)

ang-zeyu · 2022-06-10T09:21:07Z

packages/core-web/src/index.js

    return;
  }

-  window.addEventListener('resize', () => {


Just a heads up, I think will have to update the fixed-header-padding height one more time during resize.

I've the "merged" this fix from the other PR into the resizeObserver changes below

ang-zeyu · 2022-06-10T09:22:50Z

packages/core-web/src/index.js

    // reset overflow when header shows again to allow content
    // in the header such as search dropdown etc. to overflow
-    if (headerMaxHeight === '100%') {


most of the changes here (resizeObserver, this line, isHidden) are necessary due to switching to transform translate for hiding the header (height change dosen't work with position: sticky), which should be simpler (I think) to deal with too.

ang-zeyu · 2022-06-10T09:23:31Z

packages/core-web/src/styles/markbind.css

+header[sticky] {
+    position: sticky;
+    top: 0;
+    transition: transform 0.3s ease-in-out;


height transition won't work now since its sticky

damithc · 2022-06-10T09:30:53Z

Concerns:

Mainly whether the user guide is clear enough due to exposing the css variable.
https://deploy-preview-1939--markbind-master.netlify.app/userguide/tweakingthepagestructure#sticking-the-header-to-the-top

Perhaps give one more example of how the variable can be used? Having more than one example helps to identify which part of the code the user needs to change when applying to another context.

ang-zeyu · 2022-06-10T09:56:35Z

Perhaps give one more example of how the variable can be used? Having more than one example helps to identify which part of the code the user needs to change when applying to another context.

Added, though - I avoided this previously as it requires referencing some of our internal implementation details: "auto-generated, invisible page anchors". 🤔

We only have one more example use case in place otherwise which is even more obscure (setting up the header hiding transition for mobile devices).

I could also remove the tip and second example entirely and just focus on the first one to avoid confusion. Wdyt? @damithc

damithc · 2022-06-10T10:06:32Z

I could also remove the tip and second example entirely and just focus on the first one to avoid confusion. Wdyt? @damithc

Is it possible to give examples of how they can use it (rather than how we use it)? Yes, if it is unlikely users will want to use that variable, we can skip it altogether.

ang-zeyu · 2022-06-10T10:20:46Z

Is it possible to give examples of how they can use it (rather than how we use it)?

I can't think of any right now, at least. =X

Maybe @jonahtanjz @tlylt have some ideas. I can go ahead with skipping the tip + second example altogether otherwise.

Yes, if it is unlikely users will want to use that variable, we can skip it altogether.

To be clear though, its still important for the primary use case (the first example), or at least, for the author to take note of it (e.g. accidental removal) when altering layout files. Otherwise, the site / page nav will be hidden behind the header as you scroll down. Similar to the slightly awkward fixed-header-padding class currently.

ang-zeyu · 2022-06-10T14:03:16Z

Sorry, realised I haven't handled a few more implementation edge cases. Will re-ping once its ready.

Use the bounding box of the sibling element next to the sticky header to figure out if the page has scrolled past the header. As the bounding rectangle of the sticky element changes to as if it is fixed once position sticky "activates".

jonahtanjz · 2022-06-11T04:27:31Z

Maybe @jonahtanjz @tlylt have some ideas. I can go ahead with skipping the tip + second example altogether otherwise.

I don't think there are any good examples other than if the user decides to not use a site/page-nav and have their own sticky sidebar? Agree, I think it's good for the users to be aware of the use of --sticky-header-height in general if they are using sticky header. The second example on page anchors can be a little confusing to the users so I think this can be removed.

Other than that, I think the documentation looks good :)

jonahtanjz

Just noticed this, will also need to collapse the dropdown when the header is hidden on mobile. Seems like the dropdown will auto hide on blur so maybe we can trigger a blur event inside the hideHeader method?

Landing.Page.-.Google.Chrome.2022-06-12.13-29-44.mp4

tlylt

Not really related to this PR, but wondering if you guys also experience the weird behavior of having the header reappearing even if I am scrolling downwards? It happens sporadically when browsing the site on a mobile (actual phone, not on desktop inspector). I tried on iPhone Safari & Chrome and both have this effect. It contradicts the point:

To preserve screen real estate, the header is hidden on devices with a width of less than 767px when the user scrolls down, and automatically re-shown when the page is scrolled up.

...but I think it has been like this all along.

The second example on page anchors can be a little confusing to the users so I think this can be removed.

Agree that in the current state, the example is a little confusing (because it is how we use it and not how they can use it). I think either removing it or putting it in a closed panel is fine. Can't really think of any other common use cases either.

docs/userGuide/tweakingThePageStructure.md

ang-zeyu

@jonahtanjz @tlylt I've removed the second example since everyone is in agreement, and touched up the relevant parts from your reviews. Also edited the example a little.

The implementation should be ready now too :)

Just noticed this, will also need to collapse the dropdown when the header is hidden on mobile. Seems like the dropdown will auto hide on blur so maybe we can trigger a blur event inside the hideHeader method?

ouch, nice catch =X. Miscopied one of the headerSelector.css('overflow', 'hidden'); -> headerSelector.css('overflow', '').

Some more self comments to help reviewing:

ang-zeyu · 2022-06-12T07:46:06Z

docs/css/main.css

@@ -56,28 +56,28 @@ mark {
    margin: 0 auto;
    min-width: 0;
    max-width: 1000px;
+    overflow-x: auto;


Documenting the layout file changes for review + future release changelog noting (should be simplified though I think =P)

1st of 3 important changes in the layout file:

This might be somewhat controversial, but is unfortunately necessary with a sticky header. At least, I haven't found another solution.

The issue (without this) occurs on pages that overflow (e.g. "formatting contents" page or "Using html javascript and css"). The nearest scrolling ancestor which is the page / root element expands to accomodate the overflown content, which in turn does 2 things:

window.innerWidth/Height are erroneous (although there is an easy alternative for this document.documentElement.clientWidth/Height)

More important is that the way sticky positioning is defined, the sticky positioning goes haywire (with or without our header height detection)

Gif demo (open image in new tab to view in order): there are 3 settings here.

The first shows proper function of sticky positioning when the viewport zooms out to the entire page.

The second showcases how the page loads on a mobile device normally, observe how the sticky positioning completely goes haywire as the page is "zoomed" (although it dosen't quite appear like that to the reader).

The third is same as the second, except I temporarily toggled off the header hiding (the translateY) to show the issue is really unrelated to header hiding / anything we are doing.

You can try removing the overflow-x when testing this PR to see the issue better, its a little hard to present on a gif.

The fix: I've set overflow-x on the content wrapper which significantly changes ux on mobile devices (but just pages that overflow).

Pros:

As a nice side effect, this change "fixes" Fixed headers should span only the viewport space #1741.

Overflown content is still usable by swiping left / right.

Cons:

On desktop devices, although very very unlikely (since there is enough space for overflown content on mobile to fully "expand" - including our embeds), if there is overflow, I would still like to point out the horizontal scroll bar is only visible at the bottom of the content wrapper. (which can be confusing)

add to that that site development should usually be done on a desktop device (so authors will likely limit content sizes to a reasonable degree (e.g. images))

You'll no longer be able to "zoom out" on mobile (though arguably this could also be a plus for mobile ux)

Not sure if there's a better solution =, but imo its a good enough compromise.

ang-zeyu · 2022-06-12T07:47:19Z

docs/css/main.css

    position: sticky;
-    top: 0;
+    top: var(--sticky-header-height);


2nd change per the docs

ang-zeyu · 2022-06-12T07:49:44Z

docs/css/main.css

    max-width: 300px;
+    max-height: calc(100vh - var(--sticky-header-height));


last change: 100vh worked before as it took into account padding-top from fixed-header-padding.
With the shift to top: var(--sticky-header-height); the content boxes actually do shift down, so we need to minus that off.

The other changes (90vh) is just to standardise the way this is done in the layout stylesheet between the site and page nav to make things simpler.

ang-zeyu · 2022-06-12T07:52:25Z

packages/core-web/src/index.js

+       1) Bounding box is calculated as if 'position: fixed' when sticky is "activated".
+       Revert the position to 'static' temporarily to avoid this.
+
+       Seems to be harmless UX wise, even on extremely slow devices.


The change here is pretty much as explained in the comment, although I would like to get some help with testing this ^ in particular on more devices. (whether the header ever "flickers" right before it is being hidden)

Tested and works fine on my end 👍

ang-zeyu · 2022-06-12T08:02:05Z

Not really related to this PR, but wondering if you guys also experience the weird behavior of having the header reappearing even if I am scrolling downwards? It happens sporadically when browsing the site on a mobile (actual phone, not on desktop inspector). I tried on iPhone Safari & Chrome and both have this effect. It contradicts the point:

I've encountered this as well, have a hunch as to why this occuring (still is too, I think). As the debounce duration is pretty short, and touchscreens are quite sensitive (as opposed to chrome emulator operated by a mouse), there's a chance this is functioning correctly, but the change in page offset isn't "thresholded" enough. (currently we just do current > lastOffset)
Will investigate in a separate PR along with a possible ux improvement to the header hiding overall.

jonahtanjz

LGTM on my end 👍

Will merge once the temporary announcement is removed :)

jonahtanjz · 2022-06-15T16:56:16Z

@ang-zeyu I think will need you to update the commit message as well since the implementation has changed :)

ang-zeyu · 2022-06-16T04:34:10Z

@ang-zeyu I think will need you to update the commit message as well since the implementation has changed :)

Thanks for the reminder, updated

Both options

28c3358

ang-zeyu changed the title ~~Both options~~ Fix heading padding FOUC (both options 1 & 3) May 25, 2022

ang-zeyu added 3 commits May 25, 2022 16:31

Make sticky demo non-searchable

2ed7491

update tests

c3fa410

typo

597019b

ang-zeyu changed the title ~~Fix heading padding FOUC (both options 1 & 3)~~ [wip] Fix heading padding FOUC (both options 1 & 3) May 28, 2022

jonahtanjz reviewed May 28, 2022

View reviewed changes

tlylt reviewed Jun 8, 2022

View reviewed changes

ang-zeyu added 3 commits June 10, 2022 13:49

Finish mobile <header sticky>'s site / page nav

12979af

Cleanup, fix / streamline resizing logic

a91e90a

Ug edits

0b44177

ang-zeyu changed the title ~~[wip] Fix heading padding FOUC (both options 1 & 3)~~ [still wip] Fix heading padding FOUC (both options 1 & 3) Jun 10, 2022

ang-zeyu added 6 commits June 10, 2022 16:16

Sticky header only

e24c96e

Test file updates

39a6eaa

remove 1 more [fixed] selector

98485a5

some more test file updates

8a4cae8

Merge branch 'master' into fix-fixed-header-both

5eb413f

padding tweak

526f273

ang-zeyu commented Jun 10, 2022

View reviewed changes

ang-zeyu added 2 commits June 10, 2022 17:38

Remove some unnecessary padding changes from header[fixed]

41af9f5

Add another example

3728ec3

link "css variable" to mdn docs

38d5a9e

ang-zeyu changed the title ~~[still wip] Fix heading padding FOUC (both options 1 & 3)~~ Fix heading padding FOUC (both options 1 & 3) Jun 10, 2022

ang-zeyu changed the title ~~Fix heading padding FOUC (both options 1 & 3)~~ [WIP] Fix heading padding FOUC (both options 1 & 3) Jun 10, 2022

Update scrolling logic

b5672db

Use the bounding box of the sibling element next to the sticky header to figure out if the page has scrolled past the header. As the bounding rectangle of the sticky element changes to as if it is fixed once position sticky "activates".

ang-zeyu added 3 commits June 11, 2022 16:49

overflow-x for content wrapper

fd74b17

Adapt scrolling properly

a58bb21

overflow-x for content wrapper (other files+update tests)

e9f6732

jonahtanjz reviewed Jun 12, 2022

View reviewed changes

tlylt reviewed Jun 12, 2022

View reviewed changes

docs/userGuide/tweakingThePageStructure.md Outdated Show resolved Hide resolved

ang-zeyu added 3 commits June 12, 2022 15:01

Adapt left/right layout container max heights

8547c90

Fix overflow value

facad3a

Docs update

aa9f60b

ang-zeyu commented Jun 12, 2022

View reviewed changes

ang-zeyu changed the title ~~[WIP] Fix heading padding FOUC (both options 1 & 3)~~ Fix heading padding FOUC (both options 1 & 3) Jun 12, 2022

jonahtanjz approved these changes Jun 13, 2022

View reviewed changes

jonahtanjz added this to the 4.0 milestone Jun 13, 2022

Remove temporary announcement banner

b9358fb

ang-zeyu mentioned this pull request Jun 15, 2022

Tracking issue for the mobile header #1948

Open

3 tasks

ang-zeyu changed the title ~~Fix heading padding FOUC (both options 1 & 3)~~ Fix heading padding FOUC (option 3 with modifications: sticky header) Jun 15, 2022

jonahtanjz merged commit 21620cb into MarkBind:master Jun 16, 2022

jonahtanjz added the breakingChange 💥 Feature will behave significantly different, or is made obsolete label Jun 16, 2022

This was referenced Jun 18, 2022

Fix embed overflow issues #1797

Merged

Fixed headers should span only the viewport space #1741

Closed

jonahtanjz mentioned this pull request Jul 12, 2022

Auto upgrade main.css file in MarkBind sites for new releases #1961

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix heading padding FOUC (option 3 with modifications: sticky header) #1939

Fix heading padding FOUC (option 3 with modifications: sticky header) #1939

ang-zeyu commented May 25, 2022 •

edited

Loading

ang-zeyu commented May 25, 2022

damithc commented May 25, 2022

jonahtanjz May 28, 2022

tlylt left a comment

ang-zeyu commented Jun 10, 2022 •

edited

Loading

ang-zeyu left a comment

ang-zeyu Jun 10, 2022

ang-zeyu Jun 10, 2022

ang-zeyu Jun 10, 2022

damithc commented Jun 10, 2022

ang-zeyu commented Jun 10, 2022

damithc commented Jun 10, 2022

ang-zeyu commented Jun 10, 2022

ang-zeyu commented Jun 10, 2022

jonahtanjz commented Jun 11, 2022 •

edited

Loading

jonahtanjz left a comment

tlylt left a comment

ang-zeyu left a comment

ang-zeyu Jun 12, 2022

ang-zeyu Jun 12, 2022

ang-zeyu Jun 12, 2022

ang-zeyu Jun 12, 2022

jonahtanjz Jun 13, 2022

ang-zeyu commented Jun 12, 2022

jonahtanjz left a comment

jonahtanjz commented Jun 15, 2022

ang-zeyu commented Jun 16, 2022

		max-width: 300px;
		max-height: calc(100vh - var(--sticky-header-height));

Fix heading padding FOUC (option 3 with modifications: sticky header) #1939

Fix heading padding FOUC (option 3 with modifications: sticky header) #1939

Conversation

ang-zeyu commented May 25, 2022 • edited Loading

ang-zeyu commented May 25, 2022

damithc commented May 25, 2022

Choose a reason for hiding this comment

tlylt left a comment

Choose a reason for hiding this comment

ang-zeyu commented Jun 10, 2022 • edited Loading

ang-zeyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damithc commented Jun 10, 2022

ang-zeyu commented Jun 10, 2022

damithc commented Jun 10, 2022

ang-zeyu commented Jun 10, 2022

ang-zeyu commented Jun 10, 2022

jonahtanjz commented Jun 11, 2022 • edited Loading

jonahtanjz left a comment

Choose a reason for hiding this comment

tlylt left a comment

Choose a reason for hiding this comment

ang-zeyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ang-zeyu commented Jun 12, 2022

jonahtanjz left a comment

Choose a reason for hiding this comment

jonahtanjz commented Jun 15, 2022

ang-zeyu commented Jun 16, 2022

ang-zeyu commented May 25, 2022 •

edited

Loading

ang-zeyu commented Jun 10, 2022 •

edited

Loading

jonahtanjz commented Jun 11, 2022 •

edited

Loading