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

Add flush element #4141

Merged
merged 13 commits into from
May 30, 2024
Merged

Add flush element #4141

merged 13 commits into from
May 30, 2024

Conversation

bluebear94
Copy link
Contributor

See #4134.

@MDLC01
Copy link
Contributor

MDLC01 commented May 15, 2024

I think this should be mentioned on the documentation for place as well.

@tingerrr
Copy link
Contributor

Hey there, this looks great, I figured I'd give it my two cents:

I think this fits better as a sub-element of place as this primarily interacts with place(float: true) elements and nothing else. Similar to figure.caption, it makes no sense to define caption by itself.

I also think this should be included in the default heading show rule, here's my thinking:

  • there are almost no documents where a floating figure should be allowed to float past its own heading, this is a sensible default
  • we can provide set heading(flush: false) or something similar as an escape hatch, we have precedent for "off-topic" fields like these:
    • heading.outlined being only useful for outline, not heading on its own
    • figure.placement being the inclusion of said floating mechanism for convenience, not necessity, one could wrap figure in place manually
  • contrary to the point above, we could not provide this and simply tell the user to recreate the show rule, if they really don't want this
    • the downside of this approach is that adding the flush element to the show rule is trivial, while removing it is not

@bluebear94
Copy link
Contributor Author

I think this fits better as a sub-element of place as this primarily interacts with place(float: true) elements and nothing else. Similar to figure.caption, it makes no sense to define caption by itself.

I disagree. figure.caption is namespaced within figure because it’s present within a figure element. flush, on the other hand, is not (usually) used within a place element.

I also think this should be included in the default heading show rule

I agree with this, though.

@tingerrr
Copy link
Contributor

I disagree. figure.caption is namespaced within figure because it’s present within a figure element. flush, on the other hand, is not (usually) used within a place element.

I don't think this distinction matters all that much in this case, the semantic relationship is still there, a flush element on its own is useless, it's only useful when used with place(float: true) elements, a subset of place elements at large. assert.eq is also scoped within assert, but used "outside" of it, as it's not an element of course, but it is semantically related to assert (as well as other examples of function scopes).

@PgBiel
Copy link
Contributor

PgBiel commented May 18, 2024

I took a quick look, and at first I think this should work with figure.placement too (please add tests for that), which would, in theory, oppose the argument that this would exclusively affect place. However, I'm also a bit torn on whether this (relatively) small feature should warrant another name in the global namespace. Maybe it's not too bad since #4038 got merged, but worth considering.

Personally I'd be ok with something like #place.flush(), but I'd like to see other suggestions as well.

@tingerrr
Copy link
Contributor

I took a quick look, and at first I think this should work with figure.placement too (please add tests for that), which would, in theory, oppose the argument that this would exclusively affect place.

Well, the implementation of figure simply wraps itself in a place if it has a non-none placement field.

@bluebear94
Copy link
Contributor Author

Another thing to note is that ‘flush’ has another plausible meaning in the context of typesetting (‘not sticking out’), so perhaps putting it in place’s namespace might be preferable.

@laurmaedje
Copy link
Member

My first instinct was also that it should maybe be moved into the place namespace.

@laurmaedje
Copy link
Member

laurmaedje commented May 27, 2024

I also think this should be included in the default heading show rule

I'm not so sure about this. In a two-column paper, you might have lots of short sections and figures should float past them, I think.

Do we know what LaTeX does by default?

cc: LaTeX and Paper guru @Enivex

@Enivex
Copy link
Collaborator

Enivex commented May 27, 2024

I also think this should be included in the default heading show rule

I'm not so sure about this. In a two-column paper, you might have lots of short sections and figures should float past them, I think.

Do we know what LaTeX does by default?

cc: LaTeX and Paper guru @Enivex

I don't think LaTeX does anything special here. Flushing figures for every heading seems problematic to me.

@PgBiel
Copy link
Contributor

PgBiel commented May 27, 2024

I think we shouldn't force flushing on all headings by default (should be opt-in through show rules).

I additionally don't remember this being the case in LaTeX when I last used it (...which was a long while ago, so take that with a grain of salt 😄)

@tingerrr
Copy link
Contributor

Yeah, that probably makes sense, in the end it also makes opting out easier.

crates/typst/src/layout/place.rs Outdated Show resolved Hide resolved
crates/typst/src/layout/place.rs Outdated Show resolved Hide resolved
tests/suite/layout/place.typ Show resolved Hide resolved
@laurmaedje laurmaedje added this pull request to the merge queue May 30, 2024
@laurmaedje
Copy link
Member

Thanks!

Merged via the queue into typst:main with commit 23746ee May 30, 2024
6 checks passed
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.

None yet

6 participants