Skip to content
This repository was archived by the owner on Aug 29, 2025. It is now read-only.

Issue 365 - Fix tooltip position & display with fixed columns/rows#366

Merged
Marc-Andre-Rivet merged 10 commits intomasterfrom
3-issue365-fixed-tooltips
Feb 7, 2019
Merged

Issue 365 - Fix tooltip position & display with fixed columns/rows#366
Marc-Andre-Rivet merged 10 commits intomasterfrom
3-issue365-fixed-tooltips

Conversation

@Marc-Andre-Rivet
Copy link
Copy Markdown
Contributor

@Marc-Andre-Rivet Marc-Andre-Rivet commented Feb 7, 2019

Fixes #365

When the table has fixed columns/rows, the data cell can be located elsewhere than in the r1c1 portion of the table. This fix checks for the cell everywhere in the table instead.

This didn't prevent the tooltip from being is displayed for fixed cells, it only prevented adjusting its position.

@chriddyp chriddyp temporarily deployed to dash-table-review-pr-366 February 7, 2019 19:21 Inactive
"@types/ramda": "^0.25.47",
"@types/react": "^16.7.22",
"@types/react-dom": "^16.0.11",
"@types/ramda": "^0.25.48",
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

These got updated by renovate but not the py packaged version

<div className={containerClasses.join(' ')} style={tableStyle}>
<div className={classes.join(' ')} style={tableStyle}>
<div
ref='table'
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

New ref to query all table cells, whether fixed or not.

@chriddyp chriddyp temporarily deployed to dash-table-review-pr-366 February 7, 2019 19:25 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-366 February 7, 2019 19:29 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-366 February 7, 2019 20:02 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-366 February 7, 2019 20:03 Inactive
const cellBounds = table[0].getBoundingClientRect();

expect(bounds.top).to.be.greaterThan(cellBounds.top + cellBounds.height);
});
Copy link
Copy Markdown
Contributor Author

@Marc-Andre-Rivet Marc-Andre-Rivet Feb 7, 2019

Choose a reason for hiding this comment

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

This test is a bit crude and tests that the tooltip is placed below the cell. If the adjustment is not run the tooltip is in its default position and will be above the cell (top=0, left=0).

Fail run (w/o fix): https://circleci.com/gh/plotly/dash-table/5051
Successful run (w/ fix): https://circleci.com/gh/plotly/dash-table/5061

const row = tooltip.row - virtualized.offset.rows;

const { r1c1, tooltip: t } = this.refs as { [key: string]: any };
const { table, tooltip: t } = this.refs as { [key: string]: any };
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We want to query the whole table, not just the lower right fragment (which is the same as checking the whole table when nothing is fixed)

@chriddyp chriddyp temporarily deployed to dash-table-review-pr-366 February 7, 2019 20:32 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-366 February 7, 2019 21:14 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-366 February 7, 2019 21:15 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-366 February 7, 2019 21:15 Inactive
expect(bounds.top).to.be.greaterThan(cellBounds.top + cellBounds.height);
});
});
cy.wait(5000);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

oof, do we really need to wait 5 seconds and test disappearance in all of these cases?

Copy link
Copy Markdown
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

Nice fix @Marc-Andre-Rivet 💃
Just one non-blocking comment hoping to keep our total test time under control 😅

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Tooltips are positioned incorrectly with fixed rows and/or columns

3 participants