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

Issue 256, 257, 608 - Add conditional styling capabilities #729

Merged
merged 36 commits into from
Apr 21, 2020

Conversation

Marc-Andre-Rivet
Copy link
Contributor

@Marc-Andre-Rivet Marc-Andre-Rivet commented Apr 6, 2020

Closes #256
Closes #257
Closes #608

  • Replaces use of var for selected/active cells background and edge colors (IE11 fix)
  • Adds support for row_index, header_index, column_id array of values in style_**_conditional props
  • Adds support for state:'active'|'selected' condition on style_data_conditional prop

#752 for follow up selected_row support.

Marc-André Rivet added 2 commits April 6, 2020 15:39
- Partial style generation for edges (for memoization optimization)
- Additional visual tests
- changelog entry
export interface IActiveElement {
is_active?: boolean;
is_selected?: boolean;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New styling conditions

@@ -13,11 +18,11 @@ export interface IIndexedHeaderElement {
}

export interface IIndexedRowElement {
row_index?: number | 'odd' | 'even';
row_index?: number[] | number | 'odd' | 'even';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Support array of values

}

export interface INamedElement {
column_id?: ColumnId;
column_id?: ColumnId[] | ColumnId;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Support array of values

rowCondition === 'odd' ? rowIndex % 2 === 1 : rowIndex % 2 === 0;
return typeof rowCondition === 'string' ?
rowCondition === 'odd' ? rowIndex % 2 === 1 : rowIndex % 2 === 0 :
Array.isArray(rowCondition) ?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Check for array

headerCondition === 'odd' ? headerIndex % 2 === 1 : headerIndex % 2 === 0;
return typeof headerCondition === 'string' ?
headerCondition === 'odd' ? headerIndex % 2 === 1 : headerIndex % 2 === 0 :
Array.isArray(headerCondition) ?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Check for array

@@ -16,28 +18,39 @@ const partialGetter = (
) => traverseMap2(
data,
columns,
(datum, column, i) => getDataCellStyle(datum, i + offset.rows, column)(styles)
(datum, column, i) => getDataCellStyle(datum, i + offset.rows, column, false, false)(styles)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Partial getter now assume all cells are active=False, selected=False

const style = {
...SELECTED_CELL_STYLE,
...getDataCellStyle(data[i], i + offset.rows, columns[j], active, true)(styles)
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Combine default selected cell styling with calculated style

i + offset.rows,
column,
false,
false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since border calculations are now done on active cells too - need a partial calculation that assumes active=False, selected=False for all cells


R.forEach(({ row: i, column: j }) => {
const active = isActiveCell(activeCell, i, j);
const priority = active ? Number.MAX_SAFE_INTEGER : Number.MAX_SAFE_INTEGER - 1;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

active beats selected, so it gets a higher priority

@@ -4,7 +4,7 @@ import { Datum, IColumn } from 'dash-table/components/Table/props';
import { matchesDataCell, matchesDataOpCell, matchesFilterCell, getFilterOpStyles, matchesHeaderCell, getHeaderOpStyles } from 'dash-table/conditional';
import { traverse2 } from 'core/math/matrixZipMap';

function resolveEdges(styles: IConvertedStyle[]): BorderStyle {
function resolveEdges(styles: IConvertedStyle[], priority?: number): BorderStyle {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now allow for priority override - useful for the active/selected borders

@@ -14,15 +14,22 @@ function resolveEdges(styles: IConvertedStyle[]): BorderStyle {
const border = s.style[p] || s.style.border;

if (border) {
res[p] = [border, i];
res[p] = [border, priority ?? i];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

null-coalescing operator, equivalent to isNil(priority) ? i : priority

Copy link
Collaborator

Choose a reason for hiding this comment

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

Whoa, I'd never seen that before 🥇 That could simplify our lives in a million places! Can you confirm that it's properly transpiled for IE and the other browsers that don't support it?

Copy link
Contributor Author

@Marc-Andre-Rivet Marc-Andre-Rivet Apr 7, 2020

Choose a reason for hiding this comment

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

The null-coalescing operator is a new addition to ESx, and supported by TypeScript since 3.7
https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-7.html#nullish-coalescing. I've been waiting for this in JS since forever :)

This transpiles to (yes, old style void 0 for undefined!):

if (border) {
    res[p] = [border, priority !== null && priority !== void 0 ? priority : i];
}

active_cell={{ row: 1, column: 1 }}
selected_cells={[
{ row: 1, column: 1, column_id: 'b' }
]}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bunch of new visual tests. There's no need for new unit tests on the EdgesMatrix as only usage changed, no additional logic/functionality was added.

@Marc-Andre-Rivet Marc-Andre-Rivet marked this pull request as ready for review April 7, 2020 21:35
const style = {
...getDataCellStyle(data[i], i + offset.rows, columns[j], active, true)(inactiveStyles),
...SELECTED_CELL_STYLE,
...getDataCellStyle(data[i], i + offset.rows, columns[j], active, true)(activeStyles)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make an exception for active/selected, apply !active/!selected styles first, apply the baseline, then apply active/selected styles

active,
true,
priority
)(activeStyles)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same here: apply !active/!selected edges, apply default active/selected edges, then apply active/selected

@Marc-Andre-Rivet Marc-Andre-Rivet changed the title Add conditional styling capabilities Issue 256, 257, 608 - Add conditional styling capabilities Apr 16, 2020
@wbrgss
Copy link
Contributor

wbrgss commented Apr 17, 2020

@Marc-Andre-Rivet

ce95cfc verified to have fixed the issue I mentioned above (#729 (comment)), nice!

254fb2c is going to break theming for a certain project. I could use state:'active'|'selected' in the wrapper, but then it's difficult to override the theme for users. I think the best solution for me would be to override the new CSS rule in that commit in order to preserve the theming and free up state:'active'|'selected' for user overrides. But right now, that project overrides the CSS rule that uses the IE11-incompatible var(--. So this might slow down this getting released from my perspective unless I do a maintenance release 🙁

@Marc-Andre-Rivet
Copy link
Contributor Author

@wbrgss 345918a should allow you to continue using the variables for existing overrides and will allow IE11 to work with the defaults (won't be able to override through the var, but that could never work anyway)

@@ -521,6 +521,7 @@
--background-color-ellipses: rgb(253, 253, 253);
--faded-text: rgb(250, 250, 250);
--faded-text-header: rgb(180, 180, 180);
--selected-background: rgba(255, 65, 54, 0.2)
Copy link
Contributor

Choose a reason for hiding this comment

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

needs a semicolon ;)

interface ISearchParams {
get: (key: string) => string | null;
}

export default class Environment {
private static readonly _supportsCssVariables = Boolean(window.CSS?.supports?.('.some-selector', 'var(--some-var)'));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Whoa, the optional chaining operator ?. was new to me as well 🤯 🤩
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Optional_chaining

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup. Using all the new things! 😀

@alexcjohnson
Copy link
Collaborator

@wbrgss The regression is in the PR, thanks for pointing it out, it would have been a pretty nasty bug. Fixed by ce95cfc

We want the datum, not the prop

Is there a test covering this nasty bug?

@Marc-Andre-Rivet
Copy link
Contributor Author

Marc-Andre-Rivet commented Apr 21, 2020

@alexcjohnson

Is there a test covering this nasty bug?

Short answer: no.
Long answer: Added improved typing in 8350c64, should be impossible to try and use a row field as an object now.

The buggy code will now trigger a TypeScript build error:
image

Copy link
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.

💃 love it!

@Marc-Andre-Rivet Marc-Andre-Rivet merged commit 2ec0b43 into dev Apr 21, 2020
@Marc-Andre-Rivet Marc-Andre-Rivet deleted the style-active-focused-cell branch April 21, 2020 02:53
@rallyemax
Copy link

private static readonly _supportsCssVariables = Boolean(window.CSS?.supports?.('.some-selector', 'var(--some-var)'));

private static readonly _activeEdge: Edge = Environment._supportsCssVariables ? '1px solid var(--accent)' : '1px solid hotpink';

This test returns false in Chromium/Chrome 81.0.4044.129 (Linux, Win10) and Firefox 75.0. The result is that the accent color reverts to a hardcoded hotpink, preventing it from being overridden generally in CSS. (Unless I'm missing something, a general override is needed to prevent the selected cell from having accented borders, since vertically adjacent cells have adjacent borders set to this color as well and those don't have any unique class name that can be targeted with CSS.)

This works and appears to be slightly more idiomatic:

window.CSS.supports('color', 'var(--fake-var)')

That also works with other valid attributes, like the ones at issue here (border and its directional siblings).

In a more general sense, it would be nice if active edge highlighting could be toggled on and off from the Python side. If that can't be implemented, then I would kindly request that the name of the variable be unique to active edge highlighting so that it can be changed or disabled in CSS without affecting other elements (like the sort arrows).

Is this really only to support IE? I think it's worth asking the question of whether it should be done at all. If supporting a deprecated browser requires the kind of gymnastics that even might break functionality in non-deprecated browsers...well, what's the point?

@bhofmei
Copy link

bhofmei commented Jul 8, 2020

This PR really messed with my dash apps. I have numerous dash apps built on top of flask and most of the dash apps currently use data table. To keep a consistent theme across all dash apps, I was able to change the selected ground color by just creating a custom CSS and having that CSS used in all the dash apps.

With this update, it now always uses the default pink set as element style so I can't override the CSS. To keep the previous styling, I will now need to add this conditional to EVERY table. I currently have 20+ tables and many more coming.

@ghost
Copy link

ghost commented Aug 18, 2020

I am also very interested how this could be implemented in a separate css stylesheet. I found a potential solution in the community forum but it leads to an error in my case: https://community.plotly.com/t/remove-or-change-hotpink-selection-from-datatable/26168/12

Bildschirmfoto 2020-08-18 um 17 10 14

Bildschirmfoto 2020-08-18 um 17 20 03

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
dash-type-enhancement New feature or request size: 3
Projects
None yet
6 participants