search

shellscape / jsx-email

Build emails with a delightful DX
https://jsx.email
MIT License
895 stars 28 forks source link

Container does not consistently center content #176

Closed ganondev closed 4 months ago

ganondev commented 4 months ago

Expected Behavior

Content placed inside of a <Container/> is centered.

Actual Behavior

Content placed inside of a <Container/> is not automatically centered.

Additional Information

From https://jsx.email/docs/components/container:

Horizontally center child components and content

In testing, I found that the most consistent way to get this component to truly 'center' the child content is to set align="center" on the <td> inside of the container <table> rather than the table itself.

Alternatively, this might just be a documentation discrepancy, as the react-email maintainers think despite identical documentation and relatively similar component (it is a fork after all). The other way to make this work is just to set m-auto on the content, which is not terrible (though I don't know that the Container maintains much use).

shellscape commented 4 months ago

@ganondev let's resurrect your react-email PR here. I did a deep dive on where this component comes from (it's something we inherited from forking) and it points back to mj-container in MJML v3, which was removed in MJML v4 because drum roll issues with confusing users. Basically this was a half-baked implementation by react-email.

So this presents an opportunity to improve and take another step forward in this project. What I think I'd like to do is offer a boolean prop named align and set that typed to 'center' | 'left' | 'right' and default to center. This will almost certainly be a breaking change for people relying on the errant behavior, so it's a great candidate for the next major version (which is soon).

That all begs the question: is Container the right name for this component?

ganondev commented 4 months ago

Makes sense. I'm not sure about the name, but it never felt meaningful. In proper component libraries, when you have something with a vague name like container or box, the point is usually 'this is just a <div> with sensible defaults for the library', but that's not really what is happening here. It's a component for alignment, and I'd be a fan of a component that is named for what it's doing. Something like <Aligner/> but perhaps that's too narrow. It's hard to say whether this component has a lot of other use because it doesn't really do anything in its current form that Section doesn't do better.