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

Rework anchor_sections to change default behavior #1964

Merged
merged 49 commits into from
Jan 4, 2022
Merged

Conversation

cderv
Copy link
Collaborator

@cderv cderv commented Nov 26, 2020

This will close #1946

This PR is a two step process

Reverting the default

First step shared in this PR is to revert the default and use FALSE instead of TRUE. Some examples are also added in ?html_document about how to customize. The style has not changed and # is still used. I did not modified the JS - even if we don't need to check for existing anchor I believe as it will not be activated by default.

Changes:

https://github.com/rstudio/rmarkdown/pull/1964/files/f26da730418d4b62de3c72b6dd7ba0bc745a31c8

New help section:

image

Simplifying customization

Second step is experimenting with more helping way of customization. I believe we can make the choice easier for users who wants an icon instead of the dash. I have something like that in mind

output:
  html_document:
    anchor_sections:
      style: 'icon'

I don't think this would be difficult to maintain, and it would be a lot easier for helping non advanced user (who don't know CSS) to customize there outputs.

It is in progress hence the draft PR.

/cc @atusy @apreshill @yihui

@cderv
Copy link
Collaborator Author

cderv commented Nov 26, 2020

We can now do some customization easily

  • Using # is the default when TRUE
output: 
  html_document:
    anchor_sections: TRUE
  • But you could specify the style now. This would be the same.
output: 
  html_document:
    anchor_sections: 
      style: hash

And we have predefined a symbol, using \01F517\00FE0E - it will be different on where it is seen (as explained here #1946 (comment))

output: 
  html_document:
    anchor_sections: 
      style: symbol

image

and

output: 
  html_document:
    anchor_sections: 
      style: icon

which uses an svg icon
image

Advantage of the svg icon: it does not change as the symbol - always the same.
Currently, limitation: the svg icon is online and not included within the package as an svg image.

Do you think it is useful ?

Currently the implementation forces me to use several css files. I don't think it is the best, but it helps me test the API. I would have prefer to have a logic of templating but I guess that would require lua filter and pandoc template. Or ability to apply JS differently based on information in R. (I should look in bookdown maybe...)

I am thinking also of adding something to easily control the level you want to apply - only level 1 for example

output: 
  html_document:
    anchor_sections: 
      style: icon
      levels: 1

but it would definitely changing the way it works - currently it would require passing parameters from R to JS. Not sure how and if it is a good idea.

Still WIP but feedbacks are welcome.

@cderv
Copy link
Collaborator Author

cderv commented Nov 27, 2020

We could support something like this

output: 
  html_document:
    anchor_sections: 
      style: icon
      maxlevel: 1

if we revert to using a lua filter as @atusy first implementation. We could pass the value as metadata to pandoc (-M rmd_anchor_maxlevel=1) and use a lua filter like this:

local anchor_maxlevel

-- retrieve metadata
local function get_metadata(el)
  anchor_maxlevel = tonumber(el.rmd_anchor_maxlevel)
  if not anchor_maxlevel then
    anchor_maxlevel = 6
  end
end

-- insert anchor link
function insert_anchor(el)
  if el.identifier ~= "" and el.level <= anchor_maxlevel then
    el.classes:insert("hasAnchor")
    table.insert(el.content,
      pandoc.Link("", "#"..el.identifier, "", {class = "anchor-section"})
    )
  end
  return(el)
end


return {
  { Meta = get_metadata },
  { Header =  insert_anchor}
}

With the current implementation in JS, I am not sure how we could modify the levels to apply to (unless building js script from a template in a temp folder)

const h = document.querySelectorAll('h1, h2, h3, h4, h5, h6');

In the first place we switch to JS implementation because we where thinking of defaulting to FALSE and requiring checking is anchors where already set (#1884 (comment)). As we default to FALSE now this would be required anymore IMO.

Do you think this would be interesting to support this max level header to apply anchor on ?
It is less important than styling but I can see the need.

@atusy
Copy link
Collaborator

atusy commented Nov 28, 2020

@cderv I think sass will be a nice solution.

  • Generate one CSS file conditionally, instead of combining multiple CSS files conditionally.
  • Hide anchors with headers whose levels exceeds anchor_maxlevel
    • instead of modifying JS conditionally

@cderv
Copy link
Collaborator Author

cderv commented Nov 28, 2020

@atusy thanks for your feedback.

yes sass would definitely help. I just don’t want yet to introduce this dependency. But when it will be one, we could switch.

About ˋanchor_maxlevel` with the solution above there is no more JS. Only a lua filter which adds the html code and css to make the styling. Do you think this would be a useful feature or only anchor_sections for all or non makes more sense ?

If you have any comment as review on this PR, you’re welcome to share!

@apreshill
Copy link
Contributor

Hi @cderv -

I really like this approach, and the added flexibility of max level. It reminds me of the toc_depth argument too, so intuitive. And being able to pick header levels for anchors mirrors a great feature of Hugo's Goldmark rendering approach to adding anchors, see: https://gohugo.io/getting-started/configuration-markup/#heading-link-example

@cderv
Copy link
Collaborator Author

cderv commented Dec 1, 2020

In the first place we switch to JS implementation because we where thinking of defaulting to FALSE and requiring checking is anchors where already set (#1884 (comment)). As we default to FALSE now this would be required anymore IMO.

As we decided anchor_sections is now optional, I think we can avoid using the JS script that was mainly there in the first place to check is anchor section was already in use. As it is FALSE by default, it will be the user responsibility to not activate it.

Not using JS means using a Lua filter instead. That allows us to add a feature I think will be appreciated: selected the depth on which to apply the anchor on. Feedbacks from @apreshill and @mine-cetinkaya-rundel confirmed that there is some potential use case.

So a TL;DR on this PR:

  • anchor_sections: FALSE is the default now.

  • anchor_sections: TRUE will add an anchor link on all headers (h1 to h6) using the default style with a #, underlined on hover.

  • There is two levels of customization available:

    output:
      html_document:
        anchor_sections:
          style: symbol # style of the anchor ("dash", "symbol", "icon")
          depth: 2 # max depth to apply anchor on (default to max which is 6)
    • 3 styles are predifined and anchor can be set only on some levels starting by h1 header.
  • ?html_document has detailed documentation about this feature now.

  • Drawback: Anchor sections is implemented with a Lua filter, so it requires Pandoc 2.0+. As it is optional, I don't think that is an issue. And most of users should be using Pandoc 2+ already.

  • I also added some tests on all this, and I tried to factor out in a function a logic which is usually inserted inside html_document() body directly (see aee4eb9 (#1964) for this factoring-out step)

    • I think this helps for the readability of the code - maybe easier maintenance in the future
    • It clarifies test allowing to write smaller tests, closer to unit test than integration test.
      I was also thinking about tests for each feature using Rmd file to render and compare - something like knitr-example, using snapshot test, but I am not sure yet if it should live here or in its own repo. So I added on the TODO / IDEAS list for next time.
  • I also added some logic to check user inputs. rmarkdown does not use yet assertthat or any other - should we ? For now, some if condition and stop() with message are the least we could do. The aim was to help fail fast if someone does not provide the correct yaml entry for anchor_sections.

IMO this PR is ready. Reviews welcome @yihui @apreshill @atusy !

@cderv cderv marked this pull request as ready for review December 1, 2020 13:48
Copy link
Collaborator

@atusy atusy left a comment

Choose a reason for hiding this comment

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

Good job! Let me add some suggestions.
BTW, you've solved two other suggestions I was about to make (rename 'maxlevel' and refactor html_document by implementing add_anchor_secions). Nice!!

man/figures/link-black-18dp.svg Show resolved Hide resolved
@cderv
Copy link
Collaborator Author

cderv commented Dec 1, 2020

@atusy I found one drawback with the lua filter: the hasAnchor class I add to the header is in fact added by Pandoc to the div and not the header node itself because of section-divs.
That leads to all anchor sections links of the same h1 part to become visible on hover 😅 - not so bad but different behavior as before.

I guess there is no trick for that on the lua side... With JS, you were able to add hasAnchor to the parent of the added link, hence the header node.

@atusy
Copy link
Collaborator

atusy commented Dec 2, 2020

@cderv

all anchor sections links of the same h1 part to become visible on hover

Yes. That is why my initial implementation did not use the hasAnchor class.
Refer to https://github.com/atusy/rmarkdown/blob/323135ef2baea445976427865d92c9d7d19aec35/inst/rmd/h/anchor-sections/anchor-sections.css#L1-L9 .

Also, your implementation shows anchors when mouse hovers the section, not header.

@cderv cderv requested a review from yihui December 21, 2020 18:30
@cderv
Copy link
Collaborator Author

cderv commented Dec 21, 2020

BTW @jooyoungseo I added aria-label to the link. So it should work as expected when hovering the link.

@atusy
Copy link
Collaborator

atusy commented Jan 17, 2021

@cderv

I just come up with an idea to control max_level with JS.

The head argument of the htmltools::htmlDependency function defines arbitrary lines of HTML to insert into the document head.
So for example, we can add a metadata about max_level.

htmltools::htmlDependency(
  ...,
  head = sprintf("<meta itemprop='anchor-sections-max-level' content='%s'>", max_level)
)

Then, we can catch the value with JS by

document.querySelector("meta[itemprop='anchor-sections-max-level']").content

@atusy
Copy link
Collaborator

atusy commented Jan 18, 2021

Oh, the script element would be cleaner way than the meta element.
An example from MDN:

<!-- Generated by the server -->
<script id="data" type="application/json">{"userId":1234,"userName":"John Doe","memberSince":"2000-01-01T00:00:00.000Z"}</script>

<!-- Static -->
<script>
  const userInfo = JSON.parse(document.getElementById("data").text);
  console.log("User information: %o", userInfo);
</script>

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/script#embedding_data_in_html

@cderv
Copy link
Collaborator Author

cderv commented Feb 16, 2021

There is also anchorjs which is used in distill already. Did not know that one so sharing it here for later reference.

@yihui yihui self-assigned this Mar 27, 2021
@cderv cderv added the next to consider for next release label Apr 27, 2021
Copy link
Member

@yihui yihui left a comment

Choose a reason for hiding this comment

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

@cderv This PR looked more complicated than I expected, but after another review, I think it looks good overall. I guess you meant "hash" instead of "dash". Other than that, please feel free to merge it. Thanks!

@cderv cderv assigned cderv and unassigned yihui Jan 4, 2022
@cderv cderv merged commit fedcdcf into main Jan 4, 2022
@cderv cderv deleted the anchor-section-default branch January 4, 2022 19:28
@cderv cderv mentioned this pull request Mar 2, 2022
24 tasks
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
next to consider for next release
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Re-consider a more user-friendly anchor symbol, or make the default FALSE for anchor_sections
6 participants