[EuiImage] Fix empty figcaptions and non-centered EuiEmptyPrompt images#6081
[EuiImage] Fix empty figcaptions and non-centered EuiEmptyPrompt images#6081cee-chen merged 5 commits intoelastic:mainfrom
figcaptions and non-centered EuiEmptyPrompt images#6081Conversation
- max width is causing the figure to not be centered - we should set horizontal margins to `auto` if EuiEmptyPrompt is setting a custom max-width
| > * { | ||
| flex-shrink: 1; | ||
| max-width: convertToRem(360px); | ||
| margin-inline: auto; |
There was a problem hiding this comment.
To be honest, yes this fixes empty prompt's specifically, but consumers will still come across this issue in their own applications and not know to have to do this.
I, personally, think we're optimizing for a much lesser used / occurring issue (stacked floats) instead of the more common use case which is centered images via text-align.
There was a problem hiding this comment.
That's fair, but I think it depends on what we want to consider the default display type of EuiImage 🤔
- If it's
inline/inline-block(and the margin issues that come with it), sure. - But if we consider the default display to be
block(or similar, e.g. flex, table), then this fix is more valid, because EuiEmptyPrompt is setting a custom max-width, and this non-centering behavior would be the same as well fordisplay: block.
It's not super straightforward here as well as the default display value of the img tag is inline (which means it's affected by text-align), but the default display of the figure tag (which is what EuiImage uses) is block (which means it's not affected by text-align).
If we want EuiImage to behave like closer to a default img tag and the figure tag is just a semantic afterthought, then OK, I just want to make sure that's a conscious display/layout flow choice that we're making.
There was a problem hiding this comment.
After chatting about this briefly in sync, we've determined we do want EuiImage to behave closer to an img tag and not the figure tag, so I'll go ahead and revert the table change!
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6081/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6081/ |
cchaos
left a comment
There was a problem hiding this comment.
LGTM. The empty prompt's are fixed and the extra block spacing doesn't bother me.
|
Going to merge without re-running CI as latest commit was just changelog change and I want to do a backport release to get this fix into the next Kibana upgrade. |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6081/ |
Summary
closes #6080
Checklist
- [ ] Checked in both light and dark modes- [ ] Props have proper autodocs and playground toggles- [ ] Added documentation- [ ] Checked Code Sandbox works for any docs examples- [ ] Added or updated jest and cypress tests- [ ] Checked for breaking changes and labeled appropriately- [ ] Checked for accessibility including keyboard-only and screenreader modes- [ ] Updated the Figma library counterpart