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 CSS break-before, break-after, break-inside properties #2217

Merged
merged 4 commits into from Jan 12, 2023

Conversation

NiedziolkaMichal
Copy link
Member

Those are interactive examples for CSS properties break-before, break-after, break-inside, page-break-before, page-break-after, page-break-inside.

image
When button is pressed and value page or left is selected, first page looks like that:
image
Second page looks like that:
image

image

image

image

image

image

In break-inside & page-break-inside, middle box has very big height when document is in print preview. Value avoid looks like that:
image

@bsmth bsmth changed the title break-before, break-after, break-inside Add CSS break-before, break-after, break-inside properties Dec 7, 2022
@dipikabh dipikabh self-requested a review December 15, 2022 18:03
Copy link
Contributor

@dipikabh dipikabh left a comment

Choose a reason for hiding this comment

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

Apologies for the delay on getting back on this, @NiedziolkaMichal. I've added a few comments for your consideration.

Comment on lines 30 to 32
<div class="box">Content Before</div>
<div class="box" id="example-element">Content with break-before</div>
<div class="box">Content After</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

How about rephrasing to the following to add more clarity:

Suggested change
<div class="box">Content Before</div>
<div class="box" id="example-element">Content with break-before</div>
<div class="box">Content After</div>
<div class="box">Content before the property</div>
<div class="box" id="example-element">Content with 'break-before'</div>
<div class="box">Content after the property</div>

This suggestion can be applied to other html files in this pull request

Copy link
Member Author

Choose a reason for hiding this comment

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

That's better. I have applied your suggestion to all examples. Thanks

Comment on lines 9 to 21
<div class="example-choice">
<pre><code class="language-css">break-inside: avoid;</code></pre>
<button type="button" class="copy hidden" aria-hidden="true">
<span class="visually-hidden">Copy to Clipboard</span>
</button>
</div>

<div class="example-choice">
<pre><code class="language-css">break-inside: avoid-page;</code></pre>
<button type="button" class="copy hidden" aria-hidden="true">
<span class="visually-hidden">Copy to Clipboard</span>
</button>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

The description for the two values avoid and avoid-page reads quite similar and the output of this example here also does not help to clarify the difference in behavior.
Do you think filling in dummy content would help to bring out the difference between the two?

Copy link
Member Author

Choose a reason for hiding this comment

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

Those values work exactly the same way, except for the fact, that avoid is also supposed to work in a multi-column layout. In the future, we could also show how multi-column layout is affected, but currently, it's not supported well on firefox. Do you think it's better to remove one of those values?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think in that case it might be better to remove one

Copy link
Member Author

Choose a reason for hiding this comment

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

Alright, I have removed avoid-page.

Comment on lines 9 to 21
<div class="example-choice">
<pre><code class="language-css">break-before: page;</code></pre>
<button type="button" class="copy hidden" aria-hidden="true">
<span class="visually-hidden">Copy to Clipboard</span>
</button>
</div>

<div class="example-choice">
<pre><code class="language-css">break-before: left;</code></pre>
<button type="button" class="copy hidden" aria-hidden="true">
<span class="visually-hidden">Copy to Clipboard</span>
</button>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently, there is no difference visually in the print preview for page and left.

  • Would the contrast be more visible if you instead demo the page and right values? And also perhaps add one more "box-container" div if needed to highlight the difference?
  • In the print preview, can the Pages per sheet option be set to 2 to show the pages side-by-side?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am afraid that values left and right are not really working on Firefox or Chrome: https://caniuse.com/css-page-break
Should I remove them to avoid confusion?
I also don't see any way to affect the default value of Pages per sheet.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can keep the example if you think it will not cause confusion or make people wonder about the outputs. Otherwise, let's add these examples later when we're able to better demonstrate them.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's better to keep this example. I removed left values, so there will be no confusion.

Comment on lines 9 to 21
<div class="example-choice">
<pre><code class="language-css">break-after: page;</code></pre>
<button type="button" class="copy hidden" aria-hidden="true">
<span class="visually-hidden">Copy to Clipboard</span>
</button>
</div>

<div class="example-choice">
<pre><code class="language-css">break-after: left;</code></pre>
<button type="button" class="copy hidden" aria-hidden="true">
<span class="visually-hidden">Copy to Clipboard</span>
</button>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as in 'break-before' to highlight the difference in the print preview between page and left.

@dipikabh
Copy link
Contributor

Hi @NiedziolkaMichal, hope you're good. Pinging to see if you had the chance to see the comments.

@NiedziolkaMichal
Copy link
Member Author

@dipikabh Sorry for the delay. I have responded to all of your suggestions.

@dipikabh
Copy link
Contributor

Thanks, @NiedziolkaMichal! I've responded to your questions.

Copy link
Contributor

@dipikabh dipikabh left a comment

Choose a reason for hiding this comment

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

@dipikabh dipikabh merged commit 15053ff into mdn:main Jan 12, 2023
@NiedziolkaMichal
Copy link
Member Author

Thanks a lot for reviewing.

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

2 participants