Issue 295 - Header tooltips #831

Marc-Andre-Rivet · 2020-09-24T14:21:13Z

Closes #295

Adds use_with nested prop to tooltip to allow default column tooltips to be applied to either or both data and header cells (both by default)
Adds new tooltip_header prop that behaves similarly to tooltip_data but affects headers instead

Marc-Andre-Rivet · 2020-09-24T14:57:52Z

src/dash-table/components/ControlledTable/index.tsx

+                : table.querySelector(
+                      `td[data-dash-column="${id}"][data-dash-row="${row}"]:not(.phantom-cell)`
+                  );
+


Still using (row, id) but now also deciding what element to select based on header

Marc-Andre-Rivet · 2020-09-24T14:58:53Z

src/dash-table/dash/DataTable.js

@@ -858,6 +860,7 @@ export const propTypes = {
    tooltip: PropTypes.objectOf(
        PropTypes.oneOfType([
            PropTypes.exact({
+                use_with: PropTypes.oneOf(['data', 'header']),


Backward compatibility addition of use_with to the tooltip prop -- app creator can use it to define whether the default tooltip will be visible on data, headers, or both. This is for consistency and ease of use. Otherwise the only other option available would be the new tooltip_header which require specifying the tooltip for each individual column/row combination.

Can we add an explicit 'both' to go along with the implicit one, and just label that as the default? Even though it won't be set as such since it's nested, I think people would in some cases appreciate being able to set this explicitly.

Sure 71e7ce9

Marc-Andre-Rivet · 2020-09-24T15:01:36Z

src/dash-table/dash/DataTable.js

+                })
+            ])
+        )
+    ),


header equivalent of tooltip_data

Marc-Andre-Rivet · 2020-09-24T15:02:14Z

src/dash-table/derived/cell/eventHandlerProps.ts

+                        this.propsFn,
+                        rowIndex,
+                        columnIndex
+                    );


New event handlers for header cells, equivalent but slightly different from the same handlers for the data cells

Marc-Andre-Rivet · 2020-09-24T15:03:30Z

src/dash-table/derived/header/wrappers.tsx

+        labelsAndIndices: R.KeyValuePair<any[], number[]>[],
+        mergeHeaders: boolean
+    ): JSX.Element[][] =>
+        labelsAndIndices.map(([labels, indices], rowIndex) =>


Clean up. TypeScript + R.map + R.addIndex don't make a great mix, requiring explicit typing of everything in most cases. The native implementation is easier to read and is also typed.

Marc-Andre-Rivet · 2020-09-24T15:05:36Z

tests/selenium/test_tooltip.py

+    }
+
+    app = dash.Dash(__name__)
+    app.layout = DataTable(**props)


Test combination of static tooltips with/without usage and data/header tooltips to make sure usage is applied and overrides override.

Marc-Andre-Rivet · 2020-09-24T15:06:08Z

tests/selenium/conftest.py

            )
-        )[row]
+        )


Cleaned this up a bit. Thankfully this wasn't used anywhere so no test needs to be modified.

Marc-Andre-Rivet · 2020-09-24T15:07:11Z

tests/selenium/test_tooltip.py

+    cell55.move_to()
+
+    target.cell(0, "3").move_to()
+    time.sleep(1)


For the missing case, ensure the tooltip has had time to appear, just in case.

alexcjohnson · 2020-09-25T19:50:57Z

src/dash-table/dash/DataTable.js

+     * for different header columns and cells.
+     * The `property` name refers to the column ID. Each property
+     * contains a list of tooltips mapped to the table's `header`
+     * row index.


"Each property contains a list of tooltips" implies to me that we're expecting a structure:

{'col-id': ['first row', 'second row']}

rather than what I think we're actually expecting:

[{'col-id': 'first row'}, {'col-id': 'second row'}]

But the former actually feels a bit more intuitive and compact to me, even if it doesn't match tooltip_data as closely. It simplifies some simple cases: single row headers, or the same tooltip for every row, you would just provide a string or object for the value, rather than an array of strings or objects. What do you think?

Also minor point: I'd just say "The property name" rather than "The property name" - as is I was looking for something called "property" below 😅

Agreed. The current description is inaccurate and misleading.

For consistency, I don't foresee any major problem supporting both syntax (array of objects and object of arrays) for both props.

a2011e0 & e443359 & e443359

Hmm if either syntax can cover every use case, I don't think it's a good idea to support both, that'll just confuse people.

Seems to me the *_data props should all match data itself, ie arrayOf(objectOf(...)).

For tooltip_header, I mostly wanted to see if we could simplify the cases of single-row headers and multi-row headers with the same tooltip for all rows, and to do that I was thinking (1) tooltip_header would be an objectOf the row IDs, and (2) you could choose not to use an array inside that. So if all you want is simple text that applies the same to every row (of if there's only one row) you could use:

{'col1-id': 'all rows for col1', 'col2-id': 'all rows for col2'}

or eg markdown for all rows:

{'col1-id': {'type': 'markdown', 'value': '#Wow\nwhat great data!'}}

But of course you'd still need the array form when you have multiple rows and you want them to behave differently:

{ 'col1-id': ['first row', 'second row'], 'col2-id': [{'type': 'markdown', 'value': '#Big'}, {'type': 'markdown', 'value': '####small'}] }

Sure.. can update to have:

tooltip: objectOf(string | tooltip) w/ new use_with for (both,data,header)
tooltip_conditional: unchanged arrayOf(conditionalTooltip)
tooltip_data: unchanged arrayOf(objectOf(string | tooltip))
tooltip_header: new objectOf(string | tooltip | arrayOf(null|string|tooltip))`

- updated prop description

Marc-Andre-Rivet · 2020-09-29T17:57:18Z

src/dash-table/derived/table/tooltip.ts

+    const staticUseWith =
+        staticTooltip && typeof staticTooltip !== 'string'
+            ? staticTooltip.use_with
+            : TooltipUsage.Both;


Apply tooltips to both data and headers by default. This is a change from the pre-existing behavior that would have shown the tooltip only on the data cells but it seems consistent in spirit with the new functionality.

src/dash-table/derived/table/tooltip.ts

R.findLast

alexcjohnson

💃 Beautiful. Lovely tests, very easy to read and cover the cases well.

Marc-André Rivet added 2 commits September 18, 2020 17:54

Extend usage of tooltip prop to display tooltip on header cells

e6c2305

tooltip + tooltip_header + new test for header/data exclusion

d7cb741

Marc-Andre-Rivet commented Sep 24, 2020

View reviewed changes

prop description update

ffa6241

Marc-Andre-Rivet commented Sep 24, 2020

View reviewed changes

src/dash-table/dash/DataTable.js

})

])

)

),

Copy link

Contributor Author

Marc-Andre-Rivet Sep 24, 2020

Choose a reason for hiding this comment

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

header equivalent of tooltip_data

Marc-Andre-Rivet commented Sep 24, 2020

View reviewed changes

Marc-Andre-Rivet marked this pull request as ready for review September 24, 2020 15:12

Marc-Andre-Rivet requested a review from alexcjohnson as a code owner September 24, 2020 15:12

alexcjohnson reviewed Sep 25, 2020

View reviewed changes

Marc-André Rivet added 4 commits September 29, 2020 08:36

both tooltip usage

71e7ce9

update tooltip_(data|header) proptypes + test

a2011e0

- default tooltip_header value

e443359

- updated prop description

no nully

fdaabb5

Marc-Andre-Rivet commented Sep 29, 2020

View reviewed changes

Marc-André Rivet added 2 commits October 5, 2020 14:00

update props & test

23afc5a

trigger build

a80e391

Marc-Andre-Rivet requested a review from alexcjohnson October 6, 2020 21:27

alexcjohnson reviewed Oct 8, 2020

View reviewed changes

src/dash-table/derived/table/tooltip.ts Show resolved Hide resolved

R.last

455db0a

R.findLast

Marc-Andre-Rivet force-pushed the 295-header-tooltip branch from b60ec42 to 455db0a Compare October 8, 2020 18:25

alexcjohnson approved these changes Oct 8, 2020

View reviewed changes

Marc-Andre-Rivet merged commit 0fd6f61 into dev Oct 8, 2020

Marc-Andre-Rivet deleted the 295-header-tooltip branch October 8, 2020 20:40

Uh oh!

Issue 295 - Header tooltips #831

Issue 295 - Header tooltips #831

Uh oh!

Conversation

Marc-Andre-Rivet commented Sep 24, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Marc-Andre-Rivet Sep 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Marc-Andre-Rivet Sep 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Marc-Andre-Rivet Sep 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alexcjohnson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Marc-Andre-Rivet Sep 24, 2020 •

edited

Loading

Marc-Andre-Rivet Sep 24, 2020 •

edited

Loading

Marc-Andre-Rivet Sep 29, 2020 •

edited

Loading