Skip to content
This repository has been archived by the owner on Jun 25, 2020. It is now read-only.

fix: tooltip disappearance and stickiness #1

Merged
merged 1 commit into from
Mar 5, 2019

Conversation

xtinec
Copy link
Contributor

@xtinec xtinec commented Feb 20, 2019

🐛 Bug Fix
This is a replication of PR #6898 in apache/incubator-superset

Nvd3 attaches tooltips to the body of the dom, not the chart the tooltip is meant fo. On hover, it sets their opacity to 1. In order to address both their stickiness when chart reloads (PR #6805) and thier disappearance on scroll in dashboards (PR #6852), we introduce a shouldRemove parameter to hideTooltips and only remove them befor chart reloads. For the scroll events triggered on dashboards, we only hide the tooltips by setting their opacity to 0. When they get hovered over again, nvd3 sets their opacity to 1 which causes them to reappear.

@xtinec xtinec requested a review from kristw February 20, 2019 22:35
@xtinec xtinec requested a review from a team as a code owner February 20, 2019 22:35
@xtinec xtinec added the #bug Something isn't working label Feb 20, 2019
Copy link
Contributor

@williaster williaster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM I think krist is working on a build fix / you might need to rebase after that

EDIT: looks like you should remove the generator approach and simply iterate over the targets to fix the build.

@xtinec xtinec force-pushed the xtinec--fix-nvd3-tooltips branch from 78861e6 to 981c72c Compare March 5, 2019 05:30
@ghost
Copy link

ghost commented Mar 5, 2019

There were the following issues with this Pull Request

  • Commit: 981c72c
    • ✖ message may not be empty
    • ✖ type may not be empty

You may need to change the commit messages to comply with the repository contributing guidelines.


🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

@xtinec xtinec force-pushed the xtinec--fix-nvd3-tooltips branch from 981c72c to 7e3f523 Compare March 5, 2019 05:32
@ghost
Copy link

ghost commented Mar 5, 2019

There were the following issues with this Pull Request

  • Commit: 7e3f523
    • ✖ message may not be empty
    • ✖ type may not be empty

You may need to change the commit messages to comply with the repository contributing guidelines.


🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

@xtinec xtinec force-pushed the xtinec--fix-nvd3-tooltips branch 3 times, most recently from afde474 to 99247f0 Compare March 5, 2019 19:51
This is a replication of PR #6898 in apache/incubator-superset
Nvd3 attaches tooltips to the body of the dom, not the chart the tooltip is meant fo. On hover, it sets their opacity to 1. In order to address both their stickiness when chart reloads (PR #6805) and thier disappearance on scroll in dashboards (PR #6852), we introduce a shouldRemove parameter to `hideTooltips` and only remove them befor chart reloads. For the scroll events triggered on dashboards, we only hide the tooltips by setting their opacity to 0. When they get hovered over again, nvd3 sets their opacity to 1 which causes them to reappear.
@xtinec xtinec force-pushed the xtinec--fix-nvd3-tooltips branch from 99247f0 to 80b505e Compare March 5, 2019 20:03
@kristw kristw changed the title Address tooltip's disappearance and stickiness fix: tooltip disappearance and stickiness Mar 5, 2019
@kristw kristw merged commit 0cc1ea8 into master Mar 5, 2019
@delete-merged-branch delete-merged-branch bot deleted the xtinec--fix-nvd3-tooltips branch March 5, 2019 20:13
nytai referenced this pull request in preset-io/superset-ui-plugins Apr 27, 2020
[SIP-4] add lerna monorepo and`@superset-ui/core` package with `SupersetClient`
nytai referenced this pull request in preset-io/superset-ui-plugins Apr 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
#bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants