-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
UI/Truncate long secret names #13032
Conversation
👍 The tooltip behaves as expected, but is it possible to have the pointer centered on the name of the key, rather than at the end of the name? |
@@ -19,7 +19,7 @@ | |||
* @param {string} title - title of the chart | |||
* @param {array} mapLegend - array of objects with key names 'key' and 'label' for the map legend | |||
* @param {object} dataset - dataset for the chart | |||
* @param {array} tooltipData - misc. information needed to display tooltip (i.e. total clients from query params) | |||
* @param {any} tooltipData - misc. information needed to display tooltip (i.e. total clients from query params) |
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'm not sure I've seen every before. Does it come in as an object, or varies? Maybe describe / give example of types in the description?
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.
@Monkeychip I added it after seeing it somewhere else...but yeah. Going to open a separate PR that will address some tweaks this file needs.
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.
Nice. At first I was wondering about a test, but then I'm not sure if it's super relevant. I'll leave it up to you.
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.
Nice work and thanks again for adding the isTooltipCopyable property to the InfoTableRow story that I forgot!
tooltipText: text('tooltipText', 'This is tooltipText'), | ||
isTooltipCopyable: boolean('Allow tooltip to be copied', true), |
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.
Thanks for adding this for me!
if (labelText.offsetWidth > labelDiv.offsetWidth) { | ||
labelDiv.classList.add('label-overflow'); | ||
this.set('hasLabelOverflow', true); | ||
} |
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.
Nice work! didInsertElement
is a good place for this. I'm wondering if it is worthwhile to add a test to check if the label-overflow
class is applied to the element and that the tooltip appears for long labels?
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.
Thanks @zofskeez! I'd gone back and forth on a test..but I think that you're right to suggest it- this component shows up so many places it's probably worthwhile to include. I'll add!
Fixes overflow problem of long secret key values mentioned in #12962
Previous behavior:
Current behavior: