Skip to content

Conversation

@le0tan
Copy link
Contributor

@le0tan le0tan commented Apr 10, 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
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 using a plugin, post
rendering.

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.

@le0tan le0tan requested review from alyip98 and ang-zeyu April 10, 2020 12:34
@@ -0,0 +1,19 @@
const cheerio = module.parent.require('cheerio');

module.exports = {
Copy link
Contributor

Choose a reason for hiding this comment

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

don't think we should do this as a plugin since it is a core markbind feature.
You could look to insertHeaderFile and _parse to integrate this directly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can do that in postRender - the same place where plugins are applied. Will there be extra concerns by doing that?

Copy link
Contributor

Choose a reason for hiding this comment

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

mainly of code organization and performance secondly ( as putting it there means you are parsing the entire content one more time, keeping the number of parses to a minimum would be nice )

Unless absolutely necessary ( like in the case of id resolution in popovers ), I think each stage of page generation should maintain a single unit of functionality as is currently. postRender is for plugins - you could certainly add it immediately outside of postRender, but adding one entire stage of parsing just for fixed navbars seems a little bit of waste, and there is already similar parsing going on in _parse/insertHeaderFile. Would make sense to put it there from both standpoints

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we're to implement this inside _parse, fixed header would be a config option in site.json I suppose?

Copy link
Contributor

Choose a reason for hiding this comment

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

you could still 'detect' it during insertHeaderFile - though just to confirm the current implementation is <header class="header-fixed"> right? 😮

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

2 participants