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

Fix the table of content for room versions #1884

Merged
merged 5 commits into from
Jun 20, 2024

Conversation

zecakeh
Copy link
Contributor

@zecakeh zecakeh commented Jun 20, 2024

We need to use .RenderShortcodes for room version pages like in #1851, otherwise the headings in the included fragments are not present in the ToC of the page.

However, that breaks the withVersioning trick, because the output we get has shortcodes replaced by temporary opaque strings, so we cannot detect them to remove them. The solution I found is to use a page parameter in the added-in and changed-in shortcodes to detect if the current version is the same as the v parameter, since it seems to be the only way to pass a variable from the rendered page to the shortcode in the fragment.

Pull Request Checklist

Preview: https://pr1884--matrix-spec-previews.netlify.app

Like for the cs-module shortcode, use .RenderShortcodes
instead of .Content for the rver-fragment shortcode,
so the headings are detected by Hugo.

Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>
…shortcodes

Now that we use .RenderShortcodes in the rver-fragment shortcode,
we cannot remove the output of these shortcodes dynamically
because they are replaced by a temporary placeholder due to Hugo's internals.

Instead, since the `this` parameter was only used for room version,
we always use the `v` parameter and compare with the version
provided in the page's front matter.

Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>
@zecakeh zecakeh requested a review from a team as a code owner June 20, 2024 13:53
Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>
@turt2live
Copy link
Member

https://spec.matrix.org/unstable/rooms/v11/ (and others) appear to be working fine right now. Is there a place where we can see the bug?

@zecakeh
Copy link
Contributor Author

zecakeh commented Jun 20, 2024

It doesn't work fine, there are entries missing in the toc.

Example for v11:

Before this PR:

image

After this PR (notice the new entries under "State Resolution"):

image

@turt2live
Copy link
Member

oh my. That's dangerous.

Ideally we'd have CI for this, but that's not a problem for this PR to solve.

@turt2live turt2live self-requested a review June 20, 2024 15:16
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

otherwise lgtm

changelogs/room_versions/newsfragments/1884.clarification Outdated Show resolved Hide resolved
layouts/shortcodes/added-in.html Show resolved Hide resolved
@turt2live turt2live added the release-blocker Blocks the next release from happening label Jun 20, 2024
Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>
@turt2live turt2live mentioned this pull request Jun 20, 2024
19 tasks
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

Thanks!

@turt2live turt2live enabled auto-merge (squash) June 20, 2024 15:45
@turt2live turt2live merged commit 3af77f0 into matrix-org:main Jun 20, 2024
12 checks passed
@zecakeh zecakeh deleted the fix-room-versions branch June 20, 2024 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-blocker Blocks the next release from happening
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants