Skip to content

Conversation

@ang-zeyu
Copy link
Contributor

@ang-zeyu ang-zeyu commented Dec 28, 2019

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

• [x] Bug fix

What is the rationale for this request?
Currently, headings specified in markdown format ( #, ##, ... ) are correctly indexed and given an
id, enabling them to be searched.

Headings specified with pure html h1 ... h6 are not.
Let's fix this, since pure html headings can be useful in nested doms / panel slot headers.

What changes did you make? (Give an overview)

  • Add id for pure html heading tags, using the same slugify used in markdown-it-anchor
  • Update relavant test files

Provide some example code that this change will affect:

markbind-plugin-anchors.js

// Give pure html <h1..6> tags an id with the same slugify function used in markdown-it-anchor
if (!$(heading).attr('id')) {
  const slugifiedHeading = slugify($(heading).text(), { decamelize: false });
  $(heading).attr('id', slugifiedHeading);
}

$(heading).append(ANCHOR_HTML.replace('#', `#${$(heading).attr('id')}`));

Is there anything you'd like reviewers to focus on?
Would like some confirmation with #967.
( could add on the fix to this pr if its fine, since both pertain to heading indexing )

Testing instructions:
Add any <h123456> of your choice to any site. ( there's a <h1>Landing Page Title</h1> in src\template\default\index.md )

The heading should show up in the search bar, if it obeys all other search rules as well:

  • maximum heading indexing level
  • headings in default unexpanded panels and modals not indexed
  • always-index / no-index

<h123..> tags used as slot headers for panels should also now be indexed correctly when the panel has the expanded attribute

Proposed commit message: (wrap lines at 72 characters)
Fix heading indexing for pure html headings

Copy link
Member

@yamgent yamgent left a comment

Choose a reason for hiding this comment

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

Thanks for making this change. 2 things in particular:

  1. Since they can now be accessed via an id, maybe consider expanding the scope of this PR by also adding the anchor icon to these headings + allowing it to appear on page nav.
  2. Sometimes authors may want to exclude/include these headings from search index. For Markdown headings, this can be done with attributes. Possible to implement such a scenario for pure html headings?

@ang-zeyu ang-zeyu force-pushed the index-html-headings branch from 01c4468 to 7fe28b1 Compare December 30, 2019 05:32
@ang-zeyu
Copy link
Contributor Author

  1. Since they can now be accessed via an id, maybe consider expanding the scope of this PR by also adding the anchor icon to these headings + allowing it to appear on page nav.

My added code is just a "guard clause" to give headings with no id (i.e. pure html headings) an id, so that this original line adding the anchor that I only repositioned always executes
$(heading).append(ANCHOR_HTML.replace('#', `#${$(heading).attr('id')}`));

landingpagetitleanchor

  1. Sometimes authors may want to exclude/include these headings from search index. For Markdown headings, this can be done with attributes. Possible to implement such a scenario for pure html headings?

No changes were needed from the original code for this too
e.g. <h1 class="no-index">...</h1> works as expected, likewise for <h6 class="always-index">
Reason being collectHeadingsAndKeywords which processes and indexes the headings executes after page generation, during which postRender and the plugin's code is run

I realised I didn't update the section of the user guide you linked though. Thanks!

@yamgent yamgent self-requested a review December 30, 2019 05:56
Copy link
Member

@yamgent yamgent left a comment

Choose a reason for hiding this comment

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

Ah yes, my apologies, I got a bit confused just now when reading the code. Some comments:

// Give pure html <h1..6> tags an id with the same slugify function used in markdown-it-anchor
if (!$(heading).attr('id')) {
const slugifiedHeading = slugify($(heading).text(), { decamelize: false });
$(heading).attr('id', slugifiedHeading);
Copy link
Member

Choose a reason for hiding this comment

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

I realized that you placed these code on the anchor plugin's logic. If the author turns off this plugin by putting this in site.json:

"pluginsContext": {
    "anchors": {
      "off": true
    }
  }

Then the IDs for these pure html headings no longer generate (while the markdown ones still get their IDs).

With that in mind, is it possible to put this logic in Page.js instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, will move it to _parse instead.
Would it be better to also remove markdown-it-anchor entirely to standardise where the id is set?

Copy link
Member

Choose a reason for hiding this comment

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

Would it be better to also remove markdown-it-anchor entirely to standardise where the id is set?

Erm I am not sure what you meant by this question. markdown-it-anchor is a plugin to add clickable anchor icons. All heading ids will always be present regardless of whether markdown-it-anchor is enabled or not.

Copy link
Contributor Author

@ang-zeyu ang-zeyu Dec 30, 2019

Choose a reason for hiding this comment

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

markdown-it-anchor is a plugin to add clickable anchor icons. All heading ids will always be present regardless of whether markdown-it-anchor is enabled or not.

Hmm, I think that's markbind-plugin-anchors.js
Specifically this line again :
$(heading).append(ANCHOR_HTML.replace('#', `#${$(heading).attr('id')}`));

I just tried commenting this line out in lib/markdown-it/index.js
//.use(require('markdown-it-anchor'), { slugify: (str) => slugify(str, { decamelize: false }) })
anchors remain even after doing so

Enabling the permalink: true option as described in their docs would give this icon instead by default

code

heading1


markdown-it-anchor is only being used in MarkBind to provide ids to markdown headers, I think. markbind-plugin-anchors handles adding the actual ship ⚓️ icon defined using ANCHOR_HTML.

The two sound very similar haha 😅 Could have highlighted the difference more clearly, my bad

Copy link
Member

Choose a reason for hiding this comment

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

I see, thanks for the clarification. Yes let's remove markdown-it-anchor then.

## Including or Excluding Headings

**You can specify headings which are to be included or excluded from the index built by MarkBind's built-in search feature** using the `.always-index` or `.no-index` attributes.
**You can specify headings which are to be included or excluded from the index built by MarkBind's built-in search feature** using the `.always-index` or `.no-index` classes.
Copy link
Member

Choose a reason for hiding this comment

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

We don't usually call {.always-index} and {.no-index} as classes, because it does not look like one from the author's point of view. Perhaps a good compromise would be to include both (a.k.a. "attributes/classes").

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.

2 participants