-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
feat: Split tooltip for adjacent traces in two #104150
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
base: master
Are you sure you want to change the base?
Conversation
| } | ||
| )} | ||
| > | ||
| <div> |
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.
nit: Is this div needed, or would a Fragment be an alright replacement here?
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.
Great question. The div, or any other element is needed since the ButtonBar is centered by default (and the parent container is a little higher).
There are two solutions:
- Either wrap it inside a
div(or any other element) - Or update the style of
ButtonBarand use aStyledButtonBarfor this component. And "undo" thealign-items: center
I went for the first, since there is a Omit<..., 'className'> on the ButtonBar - which is maybe there for a reason.
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.
Hrrm, alrighty sounds all good to leave as is to me 👍
Lms24
left a comment
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!
| size="xs" | ||
| icon={<IconChevron direction={iconDirection} />} | ||
| aria-label={ariaLabel} | ||
| tooltipProps={tooltipProps} |
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.
l (feel free to ignore): Should we inline the toolTipProps here and just adjust message based on direction? based from what I can tell the other props are the same in both cases.
The adjacent traces did have one tooltip for both buttons. To increase UX, they were split now into two: