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
feat(toolbar): add tooltips #1766
feat(toolbar): add tooltips #1766
Conversation
✅ Deploy Preview for carbon-charts-core ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for carbon-charts-angular ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for carbon-charts-react ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
aeb9c65
to
d879004
Compare
6bfc973
to
e50106f
Compare
e50106f
to
c3a77eb
Compare
@@ -101,12 +101,26 @@ export class Toolbar extends Component { | |||
.each(function (d: any, index: number) { | |||
select(this) | |||
.select('svg') | |||
.style('pointer-events', 'none') |
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.
Disable pointer-events of svg as we only want event emitted by button.
if (this.tooltip.classed('hidden') === false) { | ||
if (this.lastTriggeredEventType !== Events.Toolbar.SHOW_TOOLTIP && | ||
this.tooltip.classed('hidden') === false | ||
) { |
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 have to keep this lastTriggeredEventType
to prevent a display issue when combining keyboard and mouse.
Can use following steps to verify on:
https://deploy-preview-1766--carbon-charts-angular.netlify.app/?path=/story/utility-truncations--truncated-labels-simple-bar
- Put to mouse over truncated label title --> See the tooltip
- Keep pressing
Tab
until one of the toolbar (e.g.: Show as table) got focus and show the tooltip. - Slightly move the mouse over truncated label title
--->
The tooltip will be shown as "Show as table", but not the label title iflastTriggeredEventType
is not used here.
@@ -110,6 +125,10 @@ export class Tooltip extends Component { | |||
|
|||
// remove the listener on chart-mouseout | |||
this.services.events.removeEventListener(Events.Chart.MOUSEOUT, this.handleHideTooltip) | |||
|
|||
// remote the listener of the toolbar |
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.
Kindly correct the spelling for remove.
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.
Thank you @RiyaJethwa.
Append commit 351a4b0 to fix it and also do a rebase.
fff8fa9
to
351a4b0
Compare
@markchou0225 thank you 🙌🏻 I think we should fix this issue there's enough room on both sides, the tooltip shouldn't really wrap into 2 lines |
Thank you @theiliad, |
174d4e4
to
cf4c417
Compare
Hi @theiliad, The tooltip is made to be with a centered-top placement. The wrapped div behavior is caused by its width not exceed to the chart holder. Tried one of the solutions in commit cf4c417 to make the toolbar tooltips always non-wrapped. The assumption is that all charts may have enough padding in user pages. Another way I can think of is to shift the toolbar to the left about 1~2rem. Can do further adjustments or even expose options in the future. 🤠 |
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.
@markchou0225 I think we're almost there. I'm just noticing another bug where if there's no room in the top of the chart, the tooltip still shows up to the top
it should really use our position util to figure out where to place itself
cf4c417
to
7ed85b1
Compare
@markchou0225 sorry for the back and fourth, but there must be a nicer way to position them. Could we maybe check if there are placements that are open, and place them centered to that placement? E.g. if top open, place in center-top, if not, bottom-center, if right is not open, left-center etc... |
7ed85b1
to
ffd19d7
Compare
noWrap: true, | ||
placements: ['top', 'bottom'] |
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.
Hi @theiliad,
Thanks for the reviewing, now we would support multiple candidates for .findBestPlacement()
to determine the best placement.
4a6741b
to
e17ac8c
Compare
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.
Sorry @markchou0225 again for the back and fourth, but could you pls just make sure that the tooltip does show up in the right place?
If I move chart to the top of the page, the tooltip still tries to show up in the top, and I can't see it
2ff29d0
to
6a83bfb
Compare
Hi @theiliad, Sorry I miss that, append commit 6a83bfb and hope that should do the trick. But as the toolbar is very close to the |
6a83bfb
to
d1ae2ab
Compare
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
thank you @markchou0225 🙌🏻 |
Closes #1549
Updates
Some testing
https://deploy-preview-1766--carbon-charts-angular.netlify.app/?path=/story/utility-truncations--truncated-labels-simple-bar
Demo screenshot or recording