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

Enable Figure to be used as a floating image. #1921

Conversation

FrenjaminBanklin
Copy link
Contributor

Closes #1882.

Added functionality to the Figure chunk to allow conversion into a floating image, which will allow text to wrap around the image.

Text is not a separate node, instead being attached to the Figure in place of the figure caption. Caption text will instead be controlled through the settings dialog, as well as which side of the text the image floats on.

…ion into a floating image, which will allow text to wrap around an image.
maufcost
maufcost previously approved these changes Oct 26, 2021
Copy link
Contributor

@maufcost maufcost left a comment

Choose a reason for hiding this comment

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

This looks pretty good to me! Amazing work. I tested wrapping texts on both left and right sides of the figure, reverting back to unwrapped texts, changing captions in the dialog when text wrapping is activated, and breaking wrapped texts into different "text" nodes.

@maufcost
Copy link
Contributor

One thing though: should this be merged into dev/26 or perhaps dev/27 (instead of directly to master)? Just in case we need to pull your changes from a specific release in the future.

@FrenjaminBanklin FrenjaminBanklin changed the base branch from master to dev/26-neon-apatite October 26, 2021 17:01
@FrenjaminBanklin FrenjaminBanklin dismissed maufcost’s stale review October 26, 2021 17:01

The base branch was changed.

@FrenjaminBanklin
Copy link
Contributor Author

I swear I targeted dev/26 when I made the PR. Oh, well.

Changed the target base - assuming all merge checks are successful, it'll require approval again.

…rap direction' with 'float direction' - left float now means right wrap and vice versa. Updated tutorial module JSON to include a demonstration of a text-wrapped image.
…disabled it for every node in the document instead of just Figure nodes - luckily alignment no longer affects text-wrapped Figure nodes anyway.
Copy link
Contributor

@maufcost maufcost left a comment

Choose a reason for hiding this comment

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

The text alignment options are working perfectly now :) Great job!

Now, don't hate me, but I found some other small issues. If you think they are too "edge-casey" or unimportant, I can go ahead and approve this since it is in a great shape so far.

  1. If you have a text wrapping around a figure and then decide not to have that text be wrapped around the image anymore, when you uncheck the "Wrap Text" checkbox, the text that was previously being wrapped is now part of the figure's caption. I think authors would be expecting that text that was being wrapped to be a text node below the image (not being wrapped anymore) when you uncheck "Wrap Text".

  2. The figure size options (medium, large, etc) are working completely fine when a figure is on its own (when there's no text wrapped around it). However, if you have some text wrapped around a figure and set the image size to medium or large, the image kind of blows up the editor with its gigantic size. Constraining the image to a fixed width or disabling the figure size options when the "Wrap Text" checkbox is checked are some options I could think of. Just for reference:

obo

@FrenjaminBanklin
Copy link
Contributor Author

Those are both pretty good points.

Point number 1 is definitely worth considering. I'll have to change the behavior to either use the new captionText property to set the caption and throw away the text, or to transform the text into a neighboring Text chunk as you mentioned.

In fact, Zach had previously done some work on this (which I only just discovered last week, because of course), but the way he was going about it made basically no alterations to the editor at all, only applying the float to the image in the viewer. That removed a lot of the potential weirdness with handling the wrapped text when enabling/disabling wrapping in the editor, since instead of being a property of the Figure itself it was just regular neighboring Text nodes. In hindsight this is probably a better approach, though I'd still like to be able to have the editor more closely reflect the final state in the viewer re: floating/wrapping.

I might even go so far as to consider point number 2 a significant enough bug to prevent merging.

Considering both together, I'm going to revert this to a draft until these two things are sorted out. If by some miracle I can get the Figure chunk to float and allow other nodes to wrap around it properly within the editor, that'd be ideal.

@FrenjaminBanklin FrenjaminBanklin marked this pull request as draft November 2, 2021 15:21
@FrenjaminBanklin
Copy link
Contributor Author

It turns out the cause for the 'image suddenly becomes enormous at medium/large size' issue is due to the caption being a single huge unbroken string. Adding a word-break: break-all; CSS rule to the figcaption element solves it.

After doing some more experimentation, I've run into a lot of the same walls Zach probably ran into with his implementation, namely that when any wrappable node is on the right side of a floating Figure, that Figure becomes unclickable because it's under an invisible wall as tall as the nodes on its right side. This issue can be resolved easily enough by giving Figure nodes a higher z-index than all potentially wrappable nodes, but then the z-index of other things needs to be made even higher to make sure things still work - the component toolbar, context buttons for List, Equation etc. chunks, and anything that generally needs to be rendered on top of the nodes below them in the document.

Another significant issue is that there's no easy way of indicating where in the document a node will appear when it's next to a floating Figure. For example, if a Figure node is float-enabled and floats to the left, and the next node on the page is a Text node, that Text node will wrap around the right side of the Figure. If the Text node is selected, the button to add a new node to the document will still appear all the way to the left, but above the Figure - although if a new node is added, it will be between the Figure and the Text. In order to actually add a new node above/before the Figure, the Figure itself has to be selected first.

On top of that, not every chunk type is really 'wrappable' - Materia, IFrame, Equation, HTML, etc. nodes just to name a few should probably always take up the full width of the document, so they'll clear any floating elements. But if one of the non-wrappable nodes is inserted between a Figure and a wrappable node, then the wrappable node will not wrap because the non-wrappable node will interrupt the Figure's float.

At the moment the least painful solution to the latter two problems is this PR's implementation, where the text that wraps around a floating image is a property of the Figure node rather than its own Text node. But the ideal solution is still to allow text to be contained in existing Text nodes and just have the Figure float to either side, so I'll continue to experiment to see if there's anything that can be done to mitigate the issues.

…unbroken single strings. Changed behavior so that when disabling text wrapping, the caption is not replaced by the wrapped text.
@stale
Copy link

stale bot commented Jun 12, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. We do this to keep our backlog under control, but we thank you for your contributions.

@stale stale bot added the stale label Jun 12, 2022
@stale stale bot closed this Jul 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrap text around images (Floating images)
2 participants