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

[v2] Fix anchor navigation in panel headers #1191

Merged
merged 6 commits into from
Apr 15, 2020

Conversation

le0tan
Copy link
Contributor

@le0tan le0tan commented Apr 11, 2020

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

• [X] Documentation update
• [X] Bug fix

Replaces #1145 #1187
Fixes #1104 Fixes #1071

What changes did you make? (Give an overview)

Move span generation to a plugin during build time. Add ::before to panel card containers to make navigation work properly on panel headers.

Provide some example code that this change will affect:

na

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

Test if the panel functions are broken in any way.

Testing instructions:

na

Proposed commit message: (wrap lines at 72 characters)

To avoid dynamically injecting lots of dummy spans in a document with
many headings, let's generate these spans at build time.

To make anchor navigation function properly on panel headers, let's
introduce ::before element to card-container to make space for the
fixed top nav.

Copy link
Contributor

@ang-zeyu ang-zeyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some nits:

src/lib/markbind/src/parser.js Outdated Show resolved Hide resolved
src/Page.js Outdated Show resolved Hide resolved
@ang-zeyu
Copy link
Contributor

Some conflicts @le0tan

@le0tan le0tan requested a review from ang-zeyu April 12, 2020 03:37
@le0tan
Copy link
Contributor Author

le0tan commented Apr 12, 2020

@ang-zeyu looks like you removed some pointer events from setupAnchors. This function is necessary in all cases as it injects the events where hovering over the heading or anchor will make the anchor icon visible. Do tell me if I misunderstood anything.

@ang-zeyu
Copy link
Contributor

@ang-zeyu looks like you removed some pointer events from setupAnchors. This function is necessary in all cases as it injects the events where hovering over the heading or anchor will make the anchor icon visible. Do tell me if I misunderstood anything.

Refer to the second commit in #1129. It's moved to plugin css ( new feature )

@le0tan
Copy link
Contributor Author

le0tan commented Apr 12, 2020

@ang-zeyu got it and I changed my code accordingly, can you take a look?

@ang-zeyu
Copy link
Contributor

@le0tan there's still a large amount of conflicts, mostly from previous versions

docs/userGuide/syntax/headers.mbdf Show resolved Hide resolved
@@ -32,20 +32,18 @@ function setupAnchorsForFixedNavbar() {
jQuery('#content-wrapper').css('padding-top', `calc(${headerHeight}px)`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it be possible to just insert the padding for the main container and not its 3 sub containers?
fixedheader

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I'm concerned it doesn't work...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, might be better to leave it this way as well: .css('nav-inner') needs to be offsetted with the original padding

navbarpadding

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just did that, wondering if we need to add 1rem to content-wrapper as well, but probably better leave it as a future first timer issue?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, would make it seem less squeezed to the top. Let's leave it as a first timer issue then, though if you want to include it here it'd be nice as well

@le0tan le0tan requested review from ang-zeyu and damithc April 14, 2020 08:26
Copy link
Contributor

@damithc damithc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but my review is from the author POV only.

@@ -32,20 +32,18 @@ function setupAnchorsForFixedNavbar() {
jQuery('#content-wrapper').css('padding-top', `calc(${headerHeight}px)`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, would make it seem less squeezed to the top. Let's leave it as a first timer issue then, though if you want to include it here it'd be nice as well

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.

Heading hidden when clicking pageNav Anchor navigation in panel not working properly with fixed header
3 participants