amp-fit-text:1.0 Vertical center non-overflowing text #32558
Conversation
| @@ -34,7 +34,7 @@ export const _default = () => { | |||
| <FitText | |||
| minFontSize={minFontSize} | |||
| maxFontSize={maxFontSize} | |||
| style={{display: 'block', border: '1px solid black', width, height}} | |||
| style={{border: '1px solid black', width, height}} | |||
There was a problem hiding this comment.
Does it matter here if display:block is set? Does it go on a wrapper or a content element?
There was a problem hiding this comment.
Yes, unfortunately it depends also whether there is overflowing text. display: block here will undo centering when there is overflowing tex (which it was doing here already), but not when there is not overflowing text (which gets vertically centered within the intermediary <div>). I guess one option is we that the middle element could take the display value from the parent element so it will impact both? But it is not intuitive.
There was a problem hiding this comment.
I think we need an extra wrapper then. I think it'd be very natural for someone to want to style this element with display: inline-block or similar.
There was a problem hiding this comment.
To clarify: You want an extra wrapper so that setting display does not impact any form of internal centering?
There was a problem hiding this comment.
Would it not suffice to encourage folks to use inline-flex? Or is the intent to stabilize the unexpected centering behavior if they do use inline-block?
There was a problem hiding this comment.
That seems pretty fragile and leaks some implementation details. If we can isolate, that'd work better, imho. E.g. centering with flexbox is one way (and maybe the best way at this time). But there could be other possibilities as well.
There was a problem hiding this comment.
Added a wrapper as suggested.
| {children} | ||
| </div> | ||
| </ContainWrapper> | ||
| <Wrapper {...rest}> |
There was a problem hiding this comment.
Any way to reverse these wrappers? It'd be very good to have contain: size layout paint on the top wrapper.
Prior to this change, the following markup would show these version differences in vertical centering for non-overflowing text:
This PR applies
flex-direction: column,justify-contents: centerto the text container inamp-fit-text, and conditionally appliesdisplay: flexto make the prior styles effective when there is not overflow, so that the text can be vertically centered within its measuring container, which in turn is already vertically centered within its outer container. Two important notes when there is overflow:Related to #28281