WordPress / twentytwentyfive

154 stars 106 forks source link

Potential problems with the flex CSS on the annotation text style #555

Closed carolinan closed 3 weeks ago

carolinan commented 1 month ago

When the annotation text style is added to a paragraph or heading that is placed directly inside the post content, not nested in other blocks,

  1. It is floating to the left of the content instead of being contained within the content width.
  2. The text alignment options in the block toolbars stops working.
  3. Left and right margin options stops working

Without the flex CSS, the border around the block extends past the length of the text content itself, but all the options do work.

I would like to ask that it is carefully considered which of these scenarios is the best and less confusing for the user.

troychaplin commented 1 month ago

I can't think of a CSS solution for this issue, and while I was sure of that I took some time to go through different options. Having a max width won't work when inline flex (or inline block) is on the same element.

Is it required to have a style for annotation on a heading? That doesn't seem to be the right element to use as an annotation. Would be much easier if it were just an option for the paragraph block, the inline property could be removed and it would just work as-is.

Image

hanneslsm commented 1 month ago

@jasmussen Why does the paragraph need to be inline-flex as you have added? When removing the css, I cannot see any breaking changes.

troychaplin commented 1 month ago

@jasmussen Why does the paragraph need to be inline-flex as you have added? When removing the css, I cannot see any breaking changes.

I'm assuming it's more to do with how a heading looks as an annotation and spanning the full width of the content area not being the desired look.

Image

jasmussen commented 4 weeks ago

Why does the paragraph need to be inline-flex as you have added?

I recommend Git Lens in VS code, it's usually better at digging up the full PR context, in this case there's additional context in #432. Troy above is right: the goal is to create a pill-shape around just the text, not the full block.

I think I have an alternate solution outlined in this codepen. GIF:

Image

This relies on the --wp--style--global--content-size to be defined, which it is in Twenty Twenty-Five. It should also work with RTL syntax out of the box, since the margin-inline is agnostic of direction. It should also work fine if you apply this style to a full paragraph, maxing out at the content width still.

jasmussen commented 4 weeks ago

CC: @juanfra @carolinan what do you think on the above solution for the annotation style?

juanfra commented 4 weeks ago

Looks really good Joen! Thank you.

I'll put that in a PR.

juanfra commented 4 weeks ago

In context, the fix is not working as expected as the editor layout relies on having the left and right margin set to auto (with !important), which is centering the annotation instead of leaving it at the left.

For the front-end, setting the margin-inline rule with the !important flag will work, but in the admin it'll get overridden. It won't be ideal to have duality between the both.

There are also some patterns where the annotation style is being used, that would be compromised as the fix changes the positioning. I believe we'll need to iterate and look for a different angle.

https://github.com/user-attachments/assets/0afddabc-9031-4ce1-a88e-f6ba14257ef2

jasmussen commented 4 weeks ago

Thank you for exploring.

In a quick test of my own, I'm not able to reproduce what you're seeing. I'm adding this as the inline CSS:

"css": "width: fit-content; margin-inline: max(0px, ((100% - var(--wp--style--global--content-size)) / 2))",

And it's working as below:

Image

Image

What nuance am I missing?

That said, if width: fit-content; works for the size of the chip, but just with the side effect that it also centers the annotation when it's inside content, perhaps that's okay too?

juanfra commented 4 weeks ago

Thanks Joen!

What nuance am I missing?

I see! I was doing the same but using !important on the margin-inline to see if we could avoid having the centering of the annotation due to the margin left/right having the !important.

That said, if width: fit-content; works for the size of the chip, but just with the side effect that it also centers the annotation when it's inside content, perhaps that's okay too?

Perfect, if we're ok with the centering then we're good with this one. I'm opening the PR (for real this time, lol)

jasmussen commented 4 weeks ago

I responded on the PR, thanks again. I'm personally in favor of keeping the !important, as it seems to work better in more situations. The centering as a fallback can work if there's a good reason.