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

feature: support customized sidebar item name from content #1825

Open
wants to merge 17 commits into
base: develop
Choose a base branch
from

Conversation

Jacco-V
Copy link

@Jacco-V Jacco-V commented Jun 22, 2022

Add support for setting a customized sidebar item name from the documentation markdown content

See also issue: #1821

Summary

This feature add support for specifying custom sidebar item names (especially for long titles) in the documentation markdown files, so that a the custom (typically shortened) item name is used in the sidebar:

## How would I write an "hello, world" example? :sidebar="hello world?"

image

What kind of change does this PR introduce?

Feature

For any code change,

  • Related documentation has been updated if needed
  • Related tests have been updated or tests have been added

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

Related issue, if any:

Tested in the following browsers:

  • Chrome
  • Firefox
  • Safari
  • Edge
  • IE

@vercel
Copy link

vercel bot commented Jun 22, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
docsify-preview ✅ Ready (Inspect) Visit Preview Sep 12, 2022 at 2:15PM (UTC)

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jun 22, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 03fda8b:

Sandbox Source
docsify-template Configuration

nextToc.slug = url;
_self.toc.push(nextToc);

return `<h${level} id="${slug}"><a href="${url}" data-id="${slug}" class="anchor"><span>${str}</span></a></h${level}>`;
return `<h${level} id="${slug}"><a href="${url}" data-id="${slug}" class="anchor"><span>${title}</span></a></h${level}>`;
Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't tried the patch, but looking at this I would think that the sidebar text would also be used in the rendered page, but I don't think this is correct. We want the sidebar text in the sidebar but not in the page.

Have you tried this? :-)

Copy link
Author

@Jacco-V Jacco-V Jun 22, 2022

Choose a reason for hiding this comment

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

No, I have not tried the changes, because I do not have a working frontend development environment. (I'm just a backend dev giving this a try).

Copy link
Author

@Jacco-V Jacco-V Jun 23, 2022

Choose a reason for hiding this comment

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

Also, where can I find the automated tests for this file?

Copy link
Member

Choose a reason for hiding this comment

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

@Jacco-V
Copy link
Author

Jacco-V commented Jun 23, 2022

I pushed some changes, but it does not work as expected :(
It seems that the :sidebar="hello world?" does not get parsed as config?

## How would I write an "hello, world" example? :sidebar="hello world?"

could it be that the config values read by getAndRemoveConfig do not support quoted strings?

https://github.com/docsifyjs/docsify/blob/1e46f2bd1dbb178ede006dbd9d05fb64e8e34b5a/src/core/render/utils.js#L21=L40

@Jacco-V
Copy link
Author

Jacco-V commented Jun 23, 2022

I've added quoted string support to the getAndRemoveConfig function and updated the tests. But I'm touching more code than I feel comfortable.

and it is kinda hard to know if things are working as expected without a working dev environment; it would be most helpful if somebody could review the changes.

@Jacco-V
Copy link
Author

Jacco-V commented Jul 3, 2022

Any chance anyone of the reviewers is going to review this?

@sy-records sy-records requested a review from a team July 3, 2022 14:30
Because the provided `str` argument has been html-escaped, with backslashes stripped, we can unfortunately not support escaped characters in quoted strings.
@Jacco-V
Copy link
Author

Jacco-V commented Jul 4, 2022

Code has been tested and is working as intended, with the sole caveat that:

  • we cannot support escaped characters in quoted-string config-values, because by the time the str is passed to the getAndRemoveConfig() method, it has been html-escaped, possibly smartypantsified, and backslashes have been stripped. However, I expect his will not be an issue for the large majority of use cases.

Changes can be seen in action in Docsify > Guide > Helpers documentation.

@Jacco-V
Copy link
Author

Jacco-V commented Aug 9, 2022

bump

@Jacco-V
Copy link
Author

Jacco-V commented Aug 28, 2022

Hi,
Is anyone going to review this PR? or is this functionality somehow not good enough?

Cheers,

Copy link
Member

@Koooooo-7 Koooooo-7 left a comment

Choose a reason for hiding this comment

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

Hi @Jacco-V , Custom sidecar item name is a good purpose.
Personally, I m considering what is the good mark to define a config content , whether we should use the " here or not . therefore I may not approve this immediately.

For the instance, based on this PR , using the mark to include content means this mark can not be used. i.e.
If using " as the mark ( :sidebar="content"), we can not use the " such as :sidebar="the include " content " like you mentioned.
Instead, if we change the mark to :sidebar={#"any content {in here}"} like:

    str = str
      .replace(
         /(?:^|\s):([\w-]+:?)=?([\w-%]+|{#.*})?/g,
        function (m, key, value) {
          if (key.indexOf(':') === -1) {
            config[key] = (value && value.replace(/{#|}$/g, '')) || true;
            return '';
          }

          return m;
        }
      )
      .replace(/^('|")|('|")$/g, '')
      .trim();

we can get this thing :
image

The point is what the mark should be the common rule/guidline for those exist/future config and it should not be very common use in user content.

Any ideas?

@Jacco-V
Copy link
Author

Jacco-V commented Aug 30, 2022

@Koooooo-7 Yes, I can see what you mean.

Architecturally speaking, it would be best if the str argument would be passed to the getAndRemoveConfig() function before the markdown has been transformed into html.

If this was the case, we could fully support quoted strings, including escape characters. This would mean you get clean code and the benefit of a using a syntax everybody knows.

:sidebar="In quoted strings we can normally use escaped \" characters"

However, I'm not sure it is possible to do this, without significant changes to the code base. Having said that, from an architectural point of view, it would still be the best option.

Another option would be to replace the regular-expression based 'parsing' with an actual parser, which would be able to handle escape charters even in the html formatted str argument. This option would get the quoted strings working, without addressing the architectural issue, but the end-user won't see the difference.

The other option could be as you propose: go for a different set of start of string and end of string markers. This would sidestep the problem. The code-changes would be more simple, at the cost introducing a custom format instead of using quoted strings and not addressing the architectural issue.

In the end, it is not up to me to choose which option is best suited for this project: I'm not one of the project members; I'm just a back-end guy who contributed a pull-request.

@Koooooo-7
Copy link
Member

@Jacco-V Thx for your input !
About change the regex to parser for configs is a good idea but we have not moved forward to it yet.
And exposing the getAndRemoveConfig as a hook is on the road. I suppose this hook could let user to custom config tpl more flexible, we could only provide a happy path one by default.

@jhildenbiddle
Copy link
Member

jhildenbiddle commented Sep 9, 2022

@Jacco-V @Koooooo-7 --

I haven't tested this PR, but perhaps we can work with the escaped character limitations using one of the following approaches:

  1. Allow titles to be wrapped in single or double quotes. This allows for either quote type to be used within the string without needing to be escaped:
    :sidebar="Single 'quotes' here"
    
    :sidebar='Double "quotes" here'
    
  2. Use HTML entities
    :sidebar='Double &quot;quotes&quot; here'
    

FWIW, I would also change the "Customize..." sidebar labels to the following:

  • Helpers > Heading IDs
  • Helpers > Sidebar Titles

These labels align with the other sidebar labels under the "Helpers". As an added bonus, it removes the "customise vs customize" localization issue. I know that's not the goal of this PR, but hey, while we're here... :)

@Koooooo-7
Copy link
Member

@jhildenbiddle How you doing ~

About the Customize changes, I think it is a nice change to make the label more plain and understandable.

The "/' approach it sounds both fine. The more importance thing to me is the now and further enrichment configs, I think we should refactor getAndRemoveConfig to be more flexible parser(s) for configs, otherwise , the regex would be more and more complex and messy.

@Jacco-V
Copy link
Author

Jacco-V commented Sep 12, 2022

@jhildenbiddle This is a possibility, and it could be an intermediate solution, but it would also be another incarnation of 'sidestepping the problem', although with a better solution than introducing custom start of string and end of string markers (in my opinion).

However, by sidestepping the problem, you continue building on non-robust legacy choice and will need to add increasingly complex regular expression(s) to hammer the non-ideal setup into a working solution. So, I agree with @Koooooo-7: Going the getAndRemoveConfig hook, with a parser way, you get more flexible, more robust, and easier to maintain code.

@jhildenbiddle
Copy link
Member

jhildenbiddle commented Sep 12, 2022

@Jacco-V, @Koooooo-7 --

No arguments from me about the issues with building upon problematic legacy code. My comment was intended only to propose a way to make the stated drawback of this PR -- the inability to support escaped characters in quoted-string config-values -- less of an issue so that we can deliver a solution sooner. If someone wants to take a crack at doing things the "better" way, I'm all for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants