-
Notifications
You must be signed in to change notification settings - Fork 60
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.
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
fix: split button overflow #1975
Conversation
✔️ Deploy preview for fundamental-styles ready! 🔨 Explore the source changes: 66635b0 🔍 Inspect the deploy logs: https://app.netlify.com/sites/fundamental-styles/deploys/5ffc7a992c16570007cb0ac7 😎 Browse the preview: https://deploy-preview-1975--fundamental-styles.netlify.app |
d4d6e9b
to
c5e763b
Compare
src/button-split.scss
Outdated
@@ -76,4 +76,16 @@ $fd-button-split-margin: 0.125rem / 2; | |||
} | |||
} | |||
} | |||
|
|||
&__text { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't you target the first child instead of adding an additional modifier class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But text is not wrapped in any element, and you cannot really target a TextNode
.
src/button-split.scss
Outdated
overflow: hidden; | ||
text-overflow: ellipsis; | ||
white-space: nowrap; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a mixin for this :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does the max-width come from? I don't see any requirements in the specs.
Shouldn't standard button component cover such cases?
stories/button/button.stories.js
Outdated
@@ -284,7 +284,7 @@ menuButton.parameters = { | |||
|
|||
export const splitMenuButton = () => ` | |||
<div class="fd-button-split fd-has-margin-right-small" role="group" aria-label="button-split"> | |||
<button class="fd-button" aria-label="button">Button with text</button> | |||
<button class="fd-button fd-button-split__text" aria-label="button">Button with a lot of text</button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you just wrap the text inside <span class="fd-button__text">Button with a lot of text</span>
?
And inside split-button just add fd-button__text { max-width: ... }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
src/button-split.scss
Outdated
@@ -76,4 +76,15 @@ $fd-button-split-margin: 0.125rem / 2; | |||
} | |||
} | |||
} | |||
|
|||
.fd-button__text { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stefanoScalzo Let's add fd-namespace
instead of fd-
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a Breaking change and should be documented
@@ -76,4 +76,15 @@ $fd-button-split-margin: 0.125rem / 2; | |||
} | |||
} | |||
} | |||
|
|||
.#{$fd-namespace}-button__text { | |||
@include fd-ellipsis(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this have a reset?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I understand what you did. You don't need the reset coz it's .fd-button__text
, but if this is the case, you can't just use it here. Split button is a separate component from Button and this violates the self-contained rule. You will need a modifier class for these rules
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
Tests are failing |
Related Issue
Closes #1721
Description
Add text overflow to the text of split button
For regular cosy button 9.75rem is used because the other button takes 2.25rem and the max-width is always 12rem
For compact 10rem is used to keep this same 12rem max-width
Screenshots
Before:
After:
Please check whether the PR fulfills the following requirements
rem
fd-*
class is used in the filefd-rtl
,fd-ellipsis
,fd-flex
,fd-selected
,fd-focus
, ect.)fd-reset()
mixin is applied to all elementsnormalize
optionunnormalize
option