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

Notify in documentation useLinkedHeadings setting change after changes from #5225 #5885

Merged
merged 3 commits into from
Mar 18, 2024

Conversation

ichim-david
Copy link
Member

No description provided.

Copy link

netlify bot commented Mar 16, 2024

Deploy Preview for plone-components canceled.

Name Link
🔨 Latest commit af2c0c1
🔍 Latest deploy log https://app.netlify.com/sites/plone-components/deploys/65f732a28e808e0008535a87

Copy link

netlify bot commented Mar 16, 2024

Deploy Preview for volto canceled.

Name Link
🔨 Latest commit af2c0c1
🔍 Latest deploy log https://app.netlify.com/sites/volto/deploys/65f732a242d8900008fd81b4

@@ -245,12 +245,12 @@ slate.runtimeDecorators = [([node, path], ranges) => ranges];

The setting `slate.useLinkedHeadings` controls whether `volto-slate` creates anchors for headings, such as `h1` and `h2`, in the editor.

The default setting is `true`.
The default setting is `false`.
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't match what I see in the code.

It creates anchor links unless the setting is explicitly equal to false:

return !token || slate.useLinkedHeadings === false ? (

So if useLinkedHeadings isn't set, it has the same effect as setting it to true.

Copy link
Member Author

Choose a reason for hiding this comment

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

This doesn't match what I see in the code.

It creates anchor links unless the setting is explicitly equal to false:

return !token || slate.useLinkedHeadings === false ? (

So if useLinkedHeadings isn't set, it has the same effect as setting it to true.

@davisagli you are correct the setting is missing, and therefore it is opt-out.
I would add a note that mentions in the documentation for this setting that it will add the anchors only
for logged in users after whatever version made those changes.
What do you think about my idea, would you add this info somewhere else?

Copy link
Member

Choose a reason for hiding this comment

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

@ichim-david yeah, it makes sense to clarify that the feature is only for editors

@ichim-david ichim-david marked this pull request as draft March 16, 2024 17:26
@ichim-david ichim-david force-pushed the 5885-linked-heading-docs branch from 6f34c7f to 0deb876 Compare March 16, 2024 17:47
@ichim-david ichim-david reopened this Mar 16, 2024
@ichim-david ichim-david changed the title Fliped useLinkedHeadings setting documentation after changes from #5225 Notify in documentation useLinkedHeadings setting change after changes from #5225 Mar 16, 2024
@ichim-david
Copy link
Member Author

@davisagli I know that we had these changes #5225 due to #5174 but it rubs me the wrong way that we make an assumption about this feature being useful only for logged-in users. If you want to enable for everyone you need to customize the render file just to remove the token condition.

What do you think about another option skipLinkedHeadlinesForAnon set to true by default?

Copy link
Collaborator

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

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

Minor grammar and vocabulary suggestions.

packages/volto/news/5885.documentation Outdated Show resolved Hide resolved
@ichim-david ichim-david marked this pull request as ready for review March 17, 2024 18:12
Copy link
Collaborator

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

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

LGTM, pending resolution of @davisagli's comment.

@davisagli davisagli merged commit 27e84b5 into main Mar 18, 2024
74 checks passed
@davisagli davisagli deleted the 5885-linked-heading-docs branch March 18, 2024 05:06
sneridagh added a commit that referenced this pull request Apr 16, 2024
* main:
  Add new types generator, don't know why they don't get included when releasing
  Release 18.0.0-alpha.22
  Release @plone/registry 1.5.3
  Cross-package manager Volto path resolver in webpack-relative-resolver (#5893)
  New `volto-update-deps` documentation (#5892)
  Release generate-volto 9.0.0-alpha.13
  Release @plone/scripts 3.6.1
  Improve and fix volto-update-deps (#5889)
  Sort facet values if they are numbers (#5865)
  Release generate-volto 9.0.0-alpha.12
  Bump @plone/scripts and @plone/types to latests (#5888)
  Release 18.0.0-alpha.21
  Release @plone/scripts 3.6.0
  Release @plone/types 1.0.0-alpha.7
  Notify in documentation useLinkedHeadings setting change after changes from #5225 (#5885)
  Add dependencies syncronizer utility (#5879)
  Update volto-slate configuration to indicate it is now part of the core. (#5886)
  Bump all the versions in GitHub workflows (#5873)
  Added download link to filename in file widget (#5880)
  Improve `@plone/types` - Block*Props - Widgets (#5876)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants