Accordion: Prevent prefix icon from shrinking at narrow widths #1002
Conversation
🦋 Changeset detectedLatest commit: c8ac5f8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
1dda109 to
a8cbe75
Compare
src/accordion.styles.ts
Outdated
| &.prefix-icon { | ||
| .default-slot { | ||
| padding-inline-start: 3.25rem; |
There was a problem hiding this comment.
Where we can, we try not to style one element via another so that each style declaration for an element is the source of truth for all its styles.
So, instead of styling .default-slot from .component, I'd recommend applying this style directly to .default-slot via a conditional class similar to .indented.
There was a problem hiding this comment.
@clintcs - Makes sense! So the .prefix-icon-slot class already existed, but wasn't being used for anything. Would you prefer I:
- Change the existing
.prefix-icon-slotto be conditional, only added if there is a prefix icon - Keep the existing
.prefix-icon-slotas-is, and add a conditional.has-prefix-iconclass - Remove the existing
.prefix-icon-slot, and only have a conditional.has-prefix-iconclass - Something else.. :)
There was a problem hiding this comment.
Check out the comment I just added. Maybe we can skirt making a decision.
There was a problem hiding this comment.
Spoke offline: we went with option 2, except changing the name of the new class to .slotted-content
src/accordion.styles.ts
Outdated
| .prefix-icon-slot { | ||
| align-items: center; | ||
| display: flex; |
There was a problem hiding this comment.
Am I missing something? Or is this and the padding above just a more indirect way of preventing the prefix icon slot from shrinking?
| display: flex; | |
| display: block; | |
| flex-shrink: 0; |
There was a problem hiding this comment.
The padding above doesn't apply here; that's on the default slot.
But yes, it looks like display: block; works (even without a flex-shrink value)
There was a problem hiding this comment.
Sorry about that. What I was getting at with the padding above was I was wondering if my suggested change above is all you need and that you could revert it and the .indentation change.
There was a problem hiding this comment.
Turns out we do need display: flex; and align-items: center, to ensure that icons are aligned with the label (tested in an application)
The indentation was existing, and I had just moved it to be nested; but I've reverted that change
a8cbe75 to
c0a38b0
Compare
|
Thanks for the fix! |
Wraps the prefix icon slot in a flex container, to prevent slotted SVGs from shrinking at narrow widths
c8ac5f8 to
c666416
Compare
Welcome back, Dan! 😆 |
🚀 Description
Wraps the Accordion prefix icon slot in a flex container, to prevent slotted SVGs from shrinking at narrow widths.
Before
After
🔬 How to Test
labeland narrow the viewport until the text starts to truncate.prefix-iconslot's slotted<glide-core-example-icon>todisplay: contents;(effectively simulates putting the<svg>directly into the slot).display: flex;from the parent.prefix-icon-slotslot element.