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

[MSKINS-226] Add custom option to explicitly enable AnchorJS along with options #51

Merged
merged 1 commit into from
Aug 15, 2024

Conversation

michael-o
Copy link
Member

@michael-o michael-o commented Apr 9, 2023

This closes #51

asfgit pushed a commit that referenced this pull request Apr 9, 2023
asfgit pushed a commit that referenced this pull request Apr 9, 2023
@hboutemy
Copy link
Member

having an option to disable, why not, but please enable by default: what is the issue with these (visual) anchors added in https://issues.apache.org/jira/browse/MSKINS-167?

@michael-o
Copy link
Member Author

michael-o commented Apr 10, 2023

having an option to disable, why not, but please enable by default: what is the issue with these (visual) anchors added in https://issues.apache.org/jira/browse/MSKINS-167?

Why I didn't enable it by default: consistency with the rest. That is:

  • Other extras aren't enable by default
  • None of the others have an explicit enable/disable flag, this would require one
  • Not everyone wants to have it

asfgit pushed a commit that referenced this pull request Apr 11, 2023
@michael-o
Copy link
Member Author

having an option to disable, why not, but please enable by default: what is the issue with these (visual) anchors added in https://issues.apache.org/jira/browse/MSKINS-167?

Why I didn't enable it by default: consistency with the rest. That is:

* Other extras aren't enable by default

* None of the others have an explicit enable/disable flag, this would require one

* Not everyone wants to have it

@hboutemy @slawekjaranowski Please let me know what you think regarding this.

@slawekjaranowski
Copy link
Member

Will be better disable it ... generated anchors ids are not the same as doxia generate

@slachiewicz
Copy link
Member

If we have working Doxia, why we need also js generated?

@michael-o
Copy link
Member Author

If we have working Doxia, why we need also js generated?

Doxia does not generate any IDs by default, you have to request it in the markup. If Doxia would generate them by default, it might lead to duplicate IDs. See here for details.

@michael-o
Copy link
Member Author

@hboutemy I will first apply the AnchorJS upgrade separately. As said, there is no way to explicitly disable this just like with other options and the inconsistency between AnchorJS IDs and Doxia IDs remains. I will leave this open for now. I still think that this is the right way since any other feature has to be enabled explicitly. This shouldn't be any different.

asfgit pushed a commit that referenced this pull request Apr 24, 2023
@michael-o michael-o changed the title [MSKINS-226] Add custom option to enable/disable AnchorJS along with … [MSKINS-226] Add custom option to explicitly enable AnchorJS along with options Apr 24, 2023
@hboutemy
Copy link
Member

hboutemy commented Jun 7, 2023

I don't understand the whole discussion about generated ids: do we care about it?

the objective is the deep anchors = the "Basic Link" in https://www.bryanbraun.com/anchorjs/
which makes sharing pointers to precise points in documentation easier

this PR makes whole things more complex: do you have any concrete issue with the current links we have in many Doxia generated sites since a few years that you want to disable it by default?

one solution to enable by default but be able to disable could be:

  <custom>
    <fluidoSkin>
      <anchorJs>
        <disable>true</disable>
      </anchorJs>
    </fluidoSkin>
  </custom>

@michael-o
Copy link
Member Author

Yes, problem is TOC macro for example. People assume that the refs in the TOC use the same id as generated by anchorjs which is not true. In fact, this skins knows nothing about Doxia. They are completely disjoint. I will need to add a note that these IDs do not correspond with Doxia IDs. Adding <disabled> is possible, but would make it inconsistent with other options.

Ref:

@michael-o
Copy link
Member Author

Added note about incompat.

asfgit pushed a commit that referenced this pull request Jun 7, 2023
asfgit pushed a commit that referenced this pull request Jun 7, 2023
asfgit pushed a commit that referenced this pull request Oct 1, 2023
@michael-o
Copy link
Member Author

Branch rebased and I still consider this reasonable to include in the next major for consistency reasons with the other custom options.

@michael-o
Copy link
Member Author

@hboutemy We have auto TOC now by @kwin done with Doxia.

asfgit pushed a commit that referenced this pull request Aug 14, 2024
@asfgit asfgit merged commit 355dd5f into master Aug 15, 2024
4 of 5 checks passed
@michael-o michael-o deleted the MSKINS-226 branch August 15, 2024 17:28
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.

5 participants