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

XRENDERING-705: Footnote macro produces inline content in standalone mode #299

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

michitux
Copy link
Contributor

@michitux michitux commented May 2, 2024

Jira URL

https://jira.xwiki.org/browse/XRENDERING-705

Changes

Description

  • "Repair" standalone footnotes by making them inline and wrapping them in a paragraph when processing them in the putFootnotes macro.
  • Adapt affected test cases.

Clarifications

  • I decided to put the repair into the putFootnotes macro as it is not possible to change the inline property of the macro block otherwise (it cannot be modified and replacing the whole macro block would break the macro transformation).
  • I decided to put the paragraph around the macro and not inside the macro (keeping it standalone) as the user experience would be bad - users probably expect to be able to drag the standalone footnote inside some text in the WYSIWYG editor, which won't be possible with a standalone footnote.

Screenshots & Video

Screencast_20240502_121725.webm

Executed Tests

mvn clean install -Pintegration-tests,docker,legacy,standalone,quality

Expected merging strategy

  • Prefers squash: Yes
  • Backport on branches:
    • stable-15.10.x

…mode

* "Repair" standalone footnotes by making them inline and wrapping them
  in a paragraph when processing them in the putFootnotes macro.
* Adapt affected test cases.
@michitux michitux requested a review from tmortagne May 2, 2024 10:27
} else {
// Wrap the macro marker block in a paragraph and make it inline.
MacroMarkerBlock result = new MacroMarkerBlock(macroMarkerBlock.getId(), macroMarkerBlock.getParameters(),
macroMarkerBlock.getContent(), macroMarkerBlock.getChildren(), true);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tmortagne I'm wondering if I should move the attributes of the block, too, any opinions on that?

Copy link
Member

@tmortagne tmortagne May 2, 2024

Choose a reason for hiding this comment

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

We don't have any attribute on MacroMarkerBlock right now AFAIK, but I feel it would be more future-proof to keep them on the new MacroMarkerBlock. We might have the same problem in other places where we do this kind of block refactoring.

That being said, shouldn't the content of a standalone footnote be standalone from the start ? It's not clear to me why it needs to be repaired.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are two things that make the footnote macro special:

  1. The initially produced content doesn't stay in the footnote but is moved to the putFootnotes macro by the putFootnotes macro and new content is set by the putFootnotes macro. So initially, the footnote macro should always produce inline content as that's what fits better in putFootnotes (it puts it inside a list item).
  2. While the new content that is added by the putFootnotes macro could be made standalone by wrapping it inside a paragraph, I felt that while technically correct, this won't provide a good user experience in the WYSIWYG editor. It doesn't make any sense for a footnote to be standalone and the WYSIWYG editor provides no way to change the footnote back to inline.

Phrased differently, what I would actually want is a way to indicate that the footnote macro doesn't support standalone mode. As this is not exactly common (I think), I did the quick solution to just wrap the footnote macro in a paragraph.

Copy link
Member

Choose a reason for hiding this comment

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

a way to indicate that the footnote macro doesn't support standalone mode

Indeed, we never thought about that need, thinking it's always easy to display something standalone if you can display it inline, but it would make sense for the footnote. I guess we either need a new Macro#supportsStandaloneMode, or deprecate #supportsInlineMode and introduce something more generic like an enum based #supportedModes (and maybe take that opportinuty to put it in the MacroDescriptor instead).

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.

2 participants