plotly / dash-table

OBSOLETE: now part of https://github.com/plotly/dash
https://dash.plotly.com
MIT License
420 stars 72 forks source link

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

Closed Marc-Andre-Rivet closed 4 years ago

Marc-Andre-Rivet commented 4 years ago

Closes #256 Closes #257 Closes #608

https://github.com/plotly/dash-table/issues/752 for follow up selected_row support.

chriddyp commented 4 years ago

@wbrgss - Could you take a look as well and make sure that we'll be able to customize some of this stuff in the Dash Enterprise Design Kit?

wbrgss commented 4 years ago

@chriddyp this looks good to me w.r.t that project. Using these new style_data_conditional props will override the theme, which IMO is what we want. After there's an npm release I'll use that to update the generate the style_data_conditional PropTypes.

wbrgss commented 4 years ago

@Marc-Andre-Rivet I'm noticing what I think is an odd regression in this PR; it could also be an unreleased regression from other changes on dev. I'm using an example you provided in the Community Forum.

On dash-table 4.6.2 this example works fine. With the built version of this PR, after clicking around to rapidly select different cells, I can't select cells anymore and I get this console error:

image

I can reproduce consistently but I'm not sure of the exact steps to get this state. Clicking more slowly seems to work OK. Please let me know if you have trouble reproducing and I can show you.

# -*- coding: utf-8 -*-
import dash
from dash_table import DataTable

import pandas as pd

url = 'https://github.com/plotly/datasets/raw/master/26k-consumer-complaints.csv'

rawDf = pd.read_csv(url)
df = rawDf.to_dict("rows"),

columns=[{"name": i, "id": i, "type": "text", 'format': {'locale': {'group': '.', 'decimal': ','}}} for i in rawDf.columns]

app = dash.Dash()
app.scripts.config.serve_locally = True

app.layout = DataTable(
    id='datatable',
    data=rawDf[0:10].to_dict("rows"),
    columns=columns,
    style_data_conditional=[
        {
            'if': {
                'filter_query': '{Sub-product} eq "Payday loan"',
            },
            'backgroundColor': 'pink'
        },
        {
            'if': {
                'filter_query': '{Sub-product} eq "Medical"',
            },
            'backgroundColor': 'lightblue'
        },
    ]
)

if __name__ == "__main__":
    app.run_server(port=8053)
Marc-Andre-Rivet commented 4 years ago

@wbrgss The regression is in the PR, thanks for pointing it out, it would have been a pretty nasty bug. Fixed by https://github.com/plotly/dash-table/pull/729/commits/ce95cfc55841d6ab82575c26b898216de62a3811

wbrgss commented 4 years ago

@Marc-Andre-Rivet

ce95cfc verified to have fixed the issue I mentioned above (https://github.com/plotly/dash-table/pull/729#issuecomment-613569260), 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 :slightly_frowning_face:

Marc-Andre-Rivet commented 4 years ago

@wbrgss https://github.com/plotly/dash-table/pull/729/commits/345918ab54d26cd51d037f5c474833ccac8e29b1 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)

alexcjohnson commented 4 years ago

@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 commented 4 years ago

@alexcjohnson

Is there a test covering this nasty bug?

Short answer: no. Long answer: Added improved typing in https://github.com/plotly/dash-table/pull/729/commits/8350c64684358f436a1bc6112db4fa0e631ae978, 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

rallyemax commented 4 years ago
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 commented 4 years ago

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 commented 4 years ago

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