fix(theme-classic): polish admonition details, render title-only admonitions #8150
Conversation
attitude
requested review from
slorber,
lex111 and
Josh-Cena
as code owners
|
Hi @attitude! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
✅ [V2]
To edit notification comments on pull requests, go to your Netlify site settings. |
Josh-Cena
added
the
pr: polish
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
website/docs/guides/markdown-features/markdown-features-admonitions.mdx
Outdated
Show resolved
Hide resolved
⚡️ Lighthouse report for the deploy preview of this PR
|
- Render admonition content `<div />` only when children are present - Apply bottom margin only when admonition content exists - Apply proper optics of the inlined SVG icon and re-calculate margins to compensate for changes - Narrow scope of `BrowserWindow` selector - Document example
Seems legit 👍 although it won't take into account children being an empty fragment, not a big deal
Also LGTM 👍
I'm not sure to understand what you mean here, can you expand considering I don't know what the rules of optics are in the first place? |
| } | ||
|
|
||
| .admonitionIcon svg { | ||
| display: inline-block; | ||
| height: 1.6em; | ||
| margin: -0.1875em; |
There was a problem hiding this comment.
what is this magic number? Can't we align icon/text without this thing? Will it work constantly with any SVG that the user provide?
Remember that users might as well provide custom SVGs through CSS, but also with swizzling, and many do this kind of customization: we don't want to make their life harder
There was a problem hiding this comment.
Seems legit 👍 although it won't take into account children being an empty fragment, not a big deal
The empty fragment will still render an empty div causing some hard time removing the bottom margin that is not necessary. That's why I changed the ternary operator around the rest.
I'm not sure to understand what you mean here, can you expand considering I don't know what the rules of optics are in the first place?
We apply negative margin in typography all the time. We push shapes like "O", "T", "V" or "A" outside of its apparent bounding box to align with other glyphs. When using custom shapes, e.g. icons alongside the text we should apply same rules.
Read more about the rules of optical alignments for example here:
- https://graphicdesign.stackexchange.com/questions/57620/aligning-letters-wrong-appears-more-right
- https://medium.muz.li/optical-effects-9fca82b4cd9a
what is this magic number? Can't we align icon/text without this thing? Will it work constantly with any SVG that the user provide?
It is because of 1.6em enlargement of icon and isn't that also a magic number too? I understand your point but it won't work without it (I've tried).
The magic number is a result of reversing the height: 1.6em rule side-effects. Height messes up line (but it can be seen only when >= 2 lines, e.g. long title on small screen or with extra large text A11Y accommodation).
Apart from fixing the extra space under the first line, pushing some of the transparent parts of SVG icons outside of the bounding box fixes the optical alignment too. (If anyone uses same construction of icons as Docusaurus does it will just work for them. And most icon libraries I have used do use some kind of a similar construction.)
I also tried emojis instead of SVGs and now they look much more well aligned.
What I try to say here is that the original code has the same "user might use some of its own SVG" flaws. What I propose is a little more optically corrected version that repositions the icon box to its expected optical centre at the 1.6em size.
If you want I could remove everything but the Fragment children and the BrowserWindow selector fixes. The rest could be done in the user land more easily after those get changed.
There was a problem hiding this comment.
The empty fragment will still render an empty div causing some hard time removing the bottom margin that is not necessary. That's why I changed the ternary operator around the rest.
Yes but see my dogfood test here:
:::note Title Only
<></>
:::It will still render an empty div in this case.
But nevermind, it's an edge case and not a big deal and the rendering look fine in both cases
There was a problem hiding this comment.
I'm not skilled enough in typography, optical alignment and all these things to say, and unfortunately I don't really have time to learn all that just for this specific PR 😅
Now, as a Docusaurus user, I want to be able to use larger svgs and fontSize for the icon, and I expect the icon / text to be "approximately" aligned by default without having to do anything with CSS
Here's a test:
https://deploy-preview-8150--docusaurus-2.netlify.app/tests/docs/tests/admonitions
## Large font icon
<admonition
type="tip"
icon={<span style={{fontSize: 60}}>💡</span>}
title="Did you know...">
<p>
content
</p>
</admonition>
<admonition
type="info"
icon={<span style={{fontSize: 40}}>ℹ️</span>}
title="Did you know...">
<p>
content
</p>
</admonition>
## Large svg icon
import InfoIcon from "@theme/Admonition/Icon/Info"
<admonition
type="tip"
icon={<InfoIcon style={{width: 60, height: 60}}/>}
title="Did you know...">
<p>
content
</p>
</admonition>
<admonition
type="info"
icon={<InfoIcon style={{width: 40, height: 40}}/>}
title="Did you know...">
<p>
content
</p>
</admonition>And by default, that does not look good to me:
I'd rather have imperfect optical alignment, but convenience for users to just increase font/svg and things work not too badly by default, rather than asking users to tweak/revert the CSS changes that you want to do here.
Does it make sense?
There was a problem hiding this comment.
Any opinion?
Maybe we could just merge the other fixes for now and eventually discuss the rest in another PR?
# Conflicts: # packages/docusaurus-theme-common/src/utils/admonitionUtils.tsx
|
Reverted some changes we couldn't agree on, good enough to merge If you want to reopen the debate on optical spacing we can do that in another PR (preferably only focusing on optical spacing) |
slorber
changed the title
slorber
added
to backport
Changes
<div />only when children are presentBrowserWindowselectorPre-flight checklist
Motivation
Fixing optics of Admonition in user-land is repetitive and could be fixed upstream for everyone.
<div>;.browserWindowBody *:last-childappliesmargin-bottom: 0too aggressively and should only apply to its direct children.Problematic elements with bounding boxes outlined:
Test Plan
Test links
Deploy preview:
Local:
Related issues/PRs