fix: base icon vertical align #10619
Conversation
web-padawan
left a comment
web-padawan
left a comment
There was a problem hiding this comment.
Tests are failing as there are some hardcoded values e.g. for ::before pseudo-element height:
❌ vaadin-icon - icon fonts > sizing > should have the same height as the host
AssertionError: expected 36 to be close to 24 +/- 1
at n.<anonymous> (packages/icon/test/icon-font.test.js:26:52)
❌ vaadin-icon - icon fonts > sizing > should not overflow host - line height
AssertionError: expected 36 to be close to 24 +/- 1
at n.<anonymous> (packages/icon/test/icon-font.test.js:47:52)
❌ vaadin-icon - icon fonts > sizing > should subtract vertical padding from height
AssertionError: expected 26 to be close to 14 +/- 1
at n.<anonymous> (packages/icon/test/icon-font.test.js:62:72)
| // https://github.com/vaadin/web-components/issues/8397 | ||
| async function iconRender(icon) { | ||
| icon.style.display = 'block'; | ||
| icon.style.setProperty('--_display-test-override', 'block'); |
There was a problem hiding this comment.
We could add something like this for testing purposes:
.block-display {
display: block !important;
}And then use icon.classList.add('block-display') and ``icon.classList.remove('block-display')` here.
There was a problem hiding this comment.
That needs to be in the shadow DOM, and then it should be :host(.block-display). Not sure if that’s any better than the custom property. Could possibly just inject extra styles into the shadow DOM in the test as well.
There was a problem hiding this comment.
I see. Yes, then we probably can add styles to shadow DOM by modifying adoptedStyleSheets. I can do that.
| ${tag}[icon-class] { | ||
| display: inline-flex; | ||
| vertical-align: middle; | ||
| font-size: inherit; |
There was a problem hiding this comment.
Removing this causes the test failures – in the test the .my-icon-font class sets font-size: 24px and line-height: 1.5 which results in a 24px * 1.5 = 36px size icon. But this class overrides the custom font icon class.
I wonder what the reason for these slotted styles was in the first place?
There was a problem hiding this comment.
I think I’d just update the tests to use the new sizes.
There was a problem hiding this comment.
Here's the relevant PR: #6418
<vaadin-icon>currently has width:var(--lumo-icon-size-m);as the default in Lumo styles.
Because of this, using a custom font size directly on
<vaadin-icon>might have an unexpected effect on the component's size. For example, the default use of Material icons would apply font-size: 24px to the element which would effectively make the icon have 36px size (= 24px * 1.5 (--lumo-icon-size-m)).
There was a problem hiding this comment.
Thanks. Okay, so we don't want font-size to be used to size the icons, ever? I did have :host { font-size: inherit !important; } for a moment, but then started wondering if sizing font icons with font size was actually desirable.
But perhaps not, and we could add the font-size override back in the shadow root. Or was the reasoning with the slotted styles so that we’d override the Material Icons case specifically, but still allow more specific selectors to override the font size?
There was a problem hiding this comment.
Now when we have --vaadin-icon-size and --vaadin-icon-visual-size properties, we can recommend using these, so maybe font-size is not very important anymore. Maybe it's ok to remove slot styles then.
There was a problem hiding this comment.
Added font-size: inherit !important.
|
Align icons on baseline instead of middle of the line-box. Remove slotted styles, always force the icon to be a flex container.
Fixes #10583 for SVG icons in all themes and for font icons in the built-in themes (Aura and Lumo).