-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Dashboard][Papercut] Customize overflow text for Links Panel #199844
Conversation
…verlfow-text-link-panel
Pinging @elastic/kibana-presentation (Team:Presentation) |
<EuiSpacer size="s" /> | ||
<EuiSwitch | ||
compressed | ||
name="overflowEllipsis" | ||
label={dashboardDrilldownConfigStrings.component.getOverflowEllipsis()} | ||
checked={options.overflowEllipsis} | ||
onChange={() => | ||
onOptionChange({ | ||
overflowEllipsis: !options.overflowEllipsis, | ||
overflowTextWrap: !options.overflowTextWrap, | ||
}) | ||
} | ||
data-test-subj="dashboardDrillDownOptions--overflowEllipsis--checkbox" | ||
/> | ||
<EuiSpacer size="s" /> | ||
<EuiSwitch | ||
compressed | ||
name="overflowTextWrap" | ||
label={dashboardDrilldownConfigStrings.component.getOverflowTextWrap()} | ||
checked={options.overflowTextWrap} | ||
onChange={() => | ||
onOptionChange({ | ||
overflowTextWrap: !options.overflowTextWrap, | ||
overflowEllipsis: !options.overflowEllipsis, | ||
}) | ||
} | ||
data-test-subj="dashboardDrillDownOptions--overflowTextWrap--checkbox" | ||
/> |
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.
@rshen91 I think it is confusing to have seperate "Wrap text" and "Ellipses" toggles - these components should be used for entirely distinct settings, and they shouldn't be "linked" (pun intended 😛) together like this:
I think a button group, where only a single option is allowed, might be a better option here. Pinging @andreadelrio here to see if she has a better suggestion, though.
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.
Good call! I updated the UI in 1ac9d8d. I'm open to suggestions with any labels and naming thanks!!!
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.
If we keep the setting per-link, I think the "Overflow" setting should come first:
Current | Suggested |
---|---|
I also wonder if we could do something with an icon instead? This is just an idea, though - would definitely need @andreadelrio's opinion here 😆
This ties in to #199844 (comment), since we shouldn't be putting this setting in the shared component - it should be just for links.
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.
+1 to using a button group.
overflowTextWrap: boolean; | ||
overflowEllipsis: boolean; |
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 this should be a setting for the entire links panel, not per-link. What do you think @rshen91?
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.
cc @andreadelrio here, too 🤔
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 personally like having this per link, for greater user flexibility, but will default to what you both think if you both like it for the entire links panel.
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.
@andreadelrio can be our tie breaker, hahaha :)
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 this should be a setting for the entire links panel.
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.
🤖 Jobs for this PR can be triggered through checkboxes. 🚧
ℹ️ To trigger the CI, please tick the checkbox below 👇
|
💔 Build Failed
Failed CI StepsHistory
cc @rshen91 |
Summary
Closes #199829
Editor toggle options added
This PR introduces a setting for the link editor panels. Users can decide to wrap text for long links or use the default clip with ellipsis.
Text wrap
Ellipses
Checklist