Make the position of top navbar fixed #982

le0tan · 2020-01-15T16:54:09Z

This should provide an alternative solution to #973, without causing too
much trouble.

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [X] Enhancement to an existing feature

What is the rationale for this request?

It's a bit strange that MarkBind doesn't fix the top navigation bar, nor re-shows the navbar when scrolled down. Moreover, currently there's no "back-to-top" button, which makes both navigating across the site and searching content a bit tricky. We can fix the top navbar to fix these problems.

What changes did you make? (Give an overview)

Changed the position of the top navbar to be fixed and move the content below down.

Replaced the vue-strap.min.js file from MarkBind/vue-strap#123

Provide some example code that this change will affect:

na

Is there anything you'd like reviewers to focus on?

na

Testing instructions:

To enable fixed header (or navbar), simply add class="header-fixed" property to the <header> element in header.md

On full width, check if the left and right page navigation div is sticking to the page correctly
On full width, check if the top navbar always shows on top of any other page content
See if the responsive design still works fine (i.e. the collapsed topnav).
Remove class="header-fixed" property to see if the navbar is no longer fixed.

Other things to notice:

whether the page nav (on the right) is working properly
whether the anchor next to the headings is working properly

Proposed commit message: (wrap lines at 72 characters)

Make top navbar position fixed

Top navbar is responsible for both navigating across the
site and placing the search bar. MarkBind often deals
with long pages thus hiding the top navbar and making
the user to go back to top to see the top navbar may not
be a good design choice.

In this PR I fixed the top navbar's position so that it's
always accessible to the user.

This should provide an alternative solution to MarkBind#973, without causing too much trouble.

damithc · 2020-01-16T01:26:42Z

Is this intended to make the top nav bar sticky? If so, ensure that scrolling to anchors works correctly. We used to have a sticky top nav in early days of MarkBind and a complication we had at that time was when we scroll to an anchor in a page, the target position of the page gets hidden behind the top nap (because the top part of the page is occupied by the top nav).

le0tan · 2020-01-16T07:09:29Z

Is this intended to make the top nav bar sticky? If so, ensure that scrolling to anchors works correctly. We used to have a sticky top nav in early days of MarkBind and a complication we had at that time was when we scroll to an anchor in a page, the target position of the page gets hidden behind the top nap (because the top part of the page is occupied by the top nav).

Thanks for the reminder! I fixed this problem in last commit =)

marvinchin

Hello! Thanks for looking at this 🙂

I've added some thoughts about the approach in the comments. Also, vue-strap.min.js should be updated independently of this PR.

asset/js/setup.js

marvinchin · 2020-01-17T00:42:57Z

asset/js/setup.js

+function setupDummySpans() {
+  jQuery('<span class="anchor"></span>').insertBefore('h1, h2, h3, h4, h5, h6');
+  jQuery('span[class="anchor"]').each((index, element) => {
+    jQuery(element).attr('id', jQuery(element).next().attr('id'));


This leads to both the dummy span and the actual heading having the same id. I think we should try to avoid this.

I removed the id for all headings (if dummy spans are added as their anchors). Is this OK?

marvinchin · 2020-01-17T00:52:01Z

asset/js/setup.js

@@ -28,6 +40,9 @@ function setupAnchors() {
                         () => jQuery(heading).find('.fa.fa-anchor').css('visibility', 'hidden'));
    }
  });
+
+  setupDummySpans();


Adding dummy spans is an interesting approach, but it does feel a little hackish. I wonder if we can make changes to the scrollToUrlAnchorHeading function to update the scrolling behaviour to do this in a cleaner way?

If this is something that you've already tried, or if you feel otherwise about that approach, feel free to bounce this back to me.

I think adding a dummy node is quite common in terms of anchoring... Above is the example in Docusarus, they also adopted this approach (only difference is they use instead of and they put the anchor inside of ).

Adding dummy spans is an interesting approach, but it does feel a little hackish. I wonder if we can make changes to the scrollToUrlAnchorHeading function to update the scrolling behaviour to do this in a cleaner way?

If I understand it correctly, isn't scrollToUrl.. only called once after the Vue instance is mounted?

That makes sense, thanks for the explanation :)

- Adopt the height offset to the actual height of <header> - Provide "fixed" prop for <navbar> component

marvinchin

Added a couple more comments!

I'm personally not a fan of dynamically injecting CSS into the page, but I'm honestly not too sure what the conventions around this are.

Maybe @Chng-Zhi-Xuan has some thoughts about this?

marvinchin · 2020-01-19T15:43:20Z

asset/js/setup.js

 function setupAnchors() {
+  const navBarSelector = jQuery('nav, .navbar');
+  const isFixed = navBarSelector.attr('style').includes('position: fixed;');


Can navBarSelector.attr('style') be undefined?

It would be undefined when the navbar is not fixed. I committed a patch on this. Thanks!

asset/js/setup.js

marvinchin · 2020-01-19T16:13:22Z

asset/js/setup.js

+        jQuery(heading).removeAttr('id'); // to avoid duplicated id problem
+        const headingHeight = jQuery(heading).height();
+        const heightOffset = navbarHeight + headingHeight;
+        const spanCss = `span#${spanId} { margin-top: calc(-${heightOffset}px - .5rem);\n`


Just wondering - why do we need to use the heading height in the calculation? Docusaurus seems to just use a static value. Would just using the height of the navbar + some clearance suffice for us? This would allow us to just use a static style instead of adding CSS dynamically like this.

I'd love to fiddle around with this a bit more to see what works, but I think the undefined attr('style') mentioned above seems to break the netlify demo. 😛 Let me know if you want me to take another look at this after you have fixed the issue.

@damithc mentioned a case where he added some extra stuff in the header: https://nus-cs2103-ay1920s1.github.io/website/index.html
In this case we have to dynamically calculate the height.

(I used a fixed number in the previous PR...

Hmm, I'm still not sure how the heading height takes into account additional things in the header. Do you have a test site with this that I can look at?

I've fiddled around with this a bit and I think this is a way we can do it without having to dynamically compute the headingHeight for each individual heading.

Move the dummy anchor span above the heading (instead of below it)

Set display: block and position: relative - we want to "float" this span above the actual heading, so that when this span is at the top of the page, our heading will be under the navbar

Set top to be negative the height of the navbar

This is what the outcome would look like:

This way, we can just use a common class for all anchors, instead of having to define a style separately for each anchor.

marvinchin

I've played with this feature a little bit, and it seems to break in some cases:

When there are additional things in the header besides the navbar (note the div with the text HELLO above the navbar):
When the window is resized:

I think it might be necessary for us to make the entire header fixed, and not just the navbar.

Let's also update the user guide for the to explain how to use the fixed navbar (or header) feature. It might also be a good idea to update one of the test sites to use this feature so that it is easier to test and catch future regressions.

le0tan · 2020-01-20T12:59:12Z

@marvinchin I requested changes to the functionality, can you take a look if it's OK? I'll get to write the documentation and tests right after your approval on the functions.

When the window is resized: ...

This problem still exists in the new solution, just much less obvious - the navbar height increases as some of the text wraps around.

marvinchin

Thanks for making the changes. I've added a couple more comments.

Also, I left a comment in an earlier comment chain about the approach for implementing the dummy anchor span but I'm not sure if you've seen it. Do you have any thoughts about that? Happy to discuss if you have any concerns.

marvinchin · 2020-01-22T06:21:27Z

asset/css/markbind.css

+header.header-fixed {
+    position: fixed;
+    width: 100%;
+    z-index: 9999;


Is there a reason why 9999 was chosen? We seem to be using z-index: 1000 for the navbar. Should we just stick to that instead?

It's just an arbitrary large integer, shouldn't matter much.

asset/js/setup.js

damithc · 2020-02-03T05:27:04Z

How's this going? Looking forward to use this feature as it can save a lot of scrolling up to the top of the web page.

yamgent · 2020-02-15T01:48:09Z

How is this PR coming along? The corresponding vue-strap PR has been waiting for a few iterations as this PR is not done yet.

le0tan · 2020-02-15T10:45:40Z

As shown in the discussion above, I’m still experimenting on the alternative implementations to minimize the performance impact. Sorry for the inconvenience caused!

On Sat, 15 Feb 2020 at 9:48 AM, Tan Wang Leng ***@***.***> wrote: How is this PR coming along? The corresponding vue-strap PR has been waiting for a few iterations as this PR is not done yet.

-- Regards, Tan Yuanhong

le0tan · 2020-02-16T09:03:08Z

Although ideally the top navbar should remain the same height in spite of the responsive layout, I did find some edge cases resulted by wrapped text:

We can mitigate such unexpected behavior by adding some extra top to it:

openorclose · 2020-02-21T03:27:01Z

http://osvaldas.info/examples/auto-hide-sticky-header/?reactive

How about this?

If user scrolls down, header disappears, when user scrolls up / clicks a link then the header appears as fixed.

le0tan · 2020-02-21T07:47:49Z

http://osvaldas.info/examples/auto-hide-sticky-header/?reactive

How about this?

If user scrolls down, header disappears, when user scrolls up / clicks a link then the header appears as fixed.

that could be another PR as another option I guess?

le0tan · 2020-02-22T02:48:13Z

@yamgent I took @marvinchin 's advice and this PR is now ready for review.

yamgent · 2020-02-22T04:14:15Z

I am not on the desktop right now, so I am trusting you that this is working fine right now 👍

The changes on the corresponding vue-strap will be ready by next release (which is hopefully tomorrow).

yamgent · 2020-02-23T02:48:02Z

...and we forgot to update the expected test files for the changes made. :P

Updated in 3953dc8.

damithc · 2020-02-23T04:29:21Z

@le0tan Does this mean the topNav should stick to the top of the viewport by default? I tried this version on CS2113 website but the topNav didn't stick.

damithc · 2020-02-27T07:52:20Z

@le0tan Does this mean the topNav should stick to the top of the viewport by default? I tried this version on CS2113 website but the topNav didn't stick.

@le0tan any updates?

* 'master' of https://github.com/MarkBind/markbind: Update tests Allow using 'none' footer attribute in frontmatter (MarkBind#1002) Support line numbers for code blocks (MarkBind#991) 2.11.0 Update test files due to changes in PR MarkBind#982 Update vue-strap version to v2.0.1-markbind.36 Make highlighting bold (MarkBind#1045) Support markdown for header attr in dropdown (MarkBind#1029) Add '_site' to the ignored folders in site.json (MarkBind#1046) Use path.join instead of string interpolation (MarkBind#1052) Implement box markdown header attributes parsing (MarkBind#1025) Make the position of top navbar fixed (MarkBind#982) Exclude *.md files from being copied over on build (MarkBind#1010) # Conflicts: # docs/css/main.css

Make the position of top navbar fixed

9244678

This should provide an alternative solution to MarkBind#973, without causing too much trouble.

le0tan requested a review from Chng-Zhi-Xuan January 15, 2020 16:54

le0tan mentioned this pull request Jan 15, 2020

Navbar: Provide options for fixed navbar MarkBind/vue-strap#123

Merged

Fix anchor problem caused by fixing navbar

d305be3

marvinchin requested changes Jan 17, 2020

View reviewed changes

le0tan added 2 commits January 19, 2020 00:01

Re-impl of fixed navbar

84fc4ce

- Adopt the height offset to the actual height of <header> - Provide "fixed" prop for <navbar> component

Revert changes on markbind.css

b12452a

le0tan requested a review from marvinchin January 18, 2020 16:04

Support different height offsets for different heading sizes

3db59af

marvinchin requested changes Jan 19, 2020

View reviewed changes

le0tan added 5 commits January 20, 2020 00:32

Fix a bug where accesses undefined's method

d82d175

Fix minor formatting issue

0ce5984

Minor fixes

1aac78e

Minor fixes

c8dc86b

Adopt changes from MarkBind/vue-strap@8a60914

9229064

le0tan requested review from marvinchin and openorclose January 20, 2020 02:58

marvinchin requested changes Jan 20, 2020

View reviewed changes

Fix the entire header using class="header-fixed"

e84c840

le0tan force-pushed the fix-top-navbar branch from 2afffd9 to e84c840 Compare January 20, 2020 12:56

le0tan requested a review from marvinchin January 20, 2020 12:59

marvinchin requested changes Jan 22, 2020

View reviewed changes

le0tan added 2 commits February 16, 2020 16:49

Implement unified dummy span css style

22346e6

Merge remote-tracking branch 'upstream/master' into fix-top-navbar

9ecfe17

le0tan requested review from marvinchin and yamgent February 16, 2020 08:53

Add height buffer to avoid extra height from wrapped text

611a2aa

yamgent approved these changes Feb 22, 2020

View reviewed changes

yamgent added this to the v2.10.1 milestone Feb 22, 2020

yamgent merged commit b900962 into MarkBind:master Feb 22, 2020

yamgent added a commit to yamgent/markbind that referenced this pull request Feb 23, 2020

Update test files due to changes in PR MarkBind#982

3953dc8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make the position of top navbar fixed #982

Make the position of top navbar fixed #982

le0tan commented Jan 15, 2020 •

edited

Loading

damithc commented Jan 16, 2020

le0tan commented Jan 16, 2020

marvinchin left a comment

marvinchin Jan 17, 2020

le0tan Jan 18, 2020

marvinchin Jan 17, 2020

le0tan Jan 18, 2020

marvinchin Jan 19, 2020

marvinchin left a comment

marvinchin Jan 19, 2020

le0tan Jan 19, 2020

marvinchin Jan 19, 2020

le0tan Jan 19, 2020

marvinchin Jan 20, 2020

marvinchin left a comment

le0tan commented Jan 20, 2020

marvinchin left a comment

marvinchin Jan 22, 2020

le0tan Feb 16, 2020

damithc commented Feb 3, 2020

yamgent commented Feb 15, 2020

le0tan commented Feb 15, 2020 via email •

edited

Loading

le0tan commented Feb 16, 2020 •

edited

Loading

openorclose commented Feb 21, 2020

le0tan commented Feb 21, 2020

le0tan commented Feb 22, 2020

yamgent commented Feb 22, 2020

yamgent commented Feb 23, 2020

damithc commented Feb 23, 2020

damithc commented Feb 27, 2020

Make the position of top navbar fixed #982

Make the position of top navbar fixed #982

Conversation

le0tan commented Jan 15, 2020 • edited Loading

damithc commented Jan 16, 2020

le0tan commented Jan 16, 2020

marvinchin 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

marvinchin 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

marvinchin left a comment

Choose a reason for hiding this comment

le0tan commented Jan 20, 2020

marvinchin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damithc commented Feb 3, 2020

yamgent commented Feb 15, 2020

le0tan commented Feb 15, 2020 via email • edited Loading

le0tan commented Feb 16, 2020 • edited Loading

openorclose commented Feb 21, 2020

le0tan commented Feb 21, 2020

le0tan commented Feb 22, 2020

yamgent commented Feb 22, 2020

yamgent commented Feb 23, 2020

damithc commented Feb 23, 2020

damithc commented Feb 27, 2020

le0tan commented Jan 15, 2020 •

edited

Loading

le0tan commented Feb 15, 2020 via email •

edited

Loading

le0tan commented Feb 16, 2020 •

edited

Loading