fix: icon baseline alignment #9661
Conversation
| @@ -72,6 +72,7 @@ class Icon extends IconMixin(ElementMixin(LumoInjectionMixin(ThemableMixin(Polyl | |||
| /** @protected */ | |||
| render() { | |||
| return html` | |||
| <span class="baseline"></span> | |||
There was a problem hiding this comment.
Could we instead covert this span to wrap <svg> and set display: contents on it e.g. like this?
.icon {
display: contents;
}
.icon::before {
content: '\\2003';
display: inline-block;
width: 0;
}There was a problem hiding this comment.
Yeah, I suppose we can. But why?
There was a problem hiding this comment.
It seems to me that dedicated wrapper would feel a bit more natural, but I'm not 100% sure.
There was a problem hiding this comment.
Note about this "hack" 🥸 #6950
There was a problem hiding this comment.
Right, thanks for bringing this up. I guess it should be ok for a standalone vaadin-icon itself but maybe it could be a problem for icon inside a button if screen readers announce that pseudo-element.
There was a problem hiding this comment.
I was more hoping for a fix if you guys try to include this in more components 😏 just searched 2003 in this repo made my realize how common this pattern is.. with such a simple fix "/ ''" available
There was a problem hiding this comment.
Right. I'd also forgotten about that issue (I think I’ve seen it before). I somehow have assumed that the extra space wouldn't cause an issue, more than a slightly longer pause in the reading. But I suppose even that is problematic.
We can at least add the content: '\2003' / '' fix, but I’m not sure how well that is supported. I remember reading that screen readers don't necessarily use it. But according to MDN, Safari 17.4 has support, which is good enough for V25: https://developer.mozilla.org/en-US/docs/Web/CSS/content#browser_compatibility
There was a problem hiding this comment.
I suggest we fix it in a separate PR for #6950.
There was a problem hiding this comment.
I added the fix for case already.
|
Icon visual test with font icons failed and the alignment does not look correct there:
|
|
Right, I didn't test that yet, so good that tests caught it. |
Should be fixed now. |
|
I’ll update the CSS version of Lumo. |
web-padawan
force-pushed
the
fix/icon-baseline-alignment
branch
from
cb726d9 to
05c271f
Compare
|
Add an explicit baseline alignment element to the shadow DOM, since both pseudo elements are already used.
Before:
After:
