Skip to content

fix(NcIconSvgWrapper): center svg span wrapper #6869

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 5, 2025
Merged

fix(NcIconSvgWrapper): center svg span wrapper #6869

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
233fded
fix(NcIconSvgWrapper): center svg span wrapper
skjnldsv May 2, 2025
f067973
fix(NcIconSvgWrapper): use `line-height` to center svg span wrapper
skjnldsv May 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/components/NcIconSvgWrapper/NcIconSvgWrapper.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,11 @@ export default {
vertical-align: text-bottom;
}

// Icon svg wrapper
span {
line-height: 0;
}
Comment on lines +289 to +291
Copy link
Contributor

@ShGKme ShGKme May 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say that the root issue is that this span is a text (inline) element, which applies the line-height.

Instead of overriding text height to 0 of a non-text text element, I'd propose to make it a block or inline-block element.

I am unsure line-height won't be overridden by some a11y extension or have other browser-specific issues.

So I'd consider it a more risky solution than making it a non-text element.

Copy link
Contributor

@susnux susnux May 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Block does not help as it still considers the line height.
Either set line height like here or use either grid (makes no sense) or flex which will fit content.

Copy link
Contributor

@ShGKme ShGKme May 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Block does not help as it still considers the line height.

Hmm, indeed

flex which will fit content.

Yep, flex/inline-flex/grid works fine.

Or display: contents

makes no sense

IMO, it makes more sense, as it sets a more relevant box type, than changing line-height of non-text 👀

Copy link
Contributor

@susnux susnux May 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or display: contents

This seems to be the ideal solution, totally forgot about that.


&:deep(svg) {
fill: currentColor;
width: v-bind('iconSize');
Expand Down
Loading