Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable DDK compatibility #1541

Merged
merged 3 commits into from
May 2, 2019
Merged

Enable DDK compatibility #1541

merged 3 commits into from
May 2, 2019

Conversation

wbrgss
Copy link
Contributor

@wbrgss wbrgss commented Apr 30, 2019

In Dash Design Kit, we allow for colors to be specified with var(--*) syntax, similar to CSS variable syntax. This will allow for Graph colors to dynamically match theme colors when the theme is changed with the Theme Editor.

When using plotly.py to generate Graph data, a ValueError is raised when using this syntax:

 ValueError: 
 Invalid value of type 'builtins.str' received for the 'color' property of scatter.marker
 Received value: 'var(--colorway-1)'

This crashes the Dash app, despite the fact that DDK will safely evaluate colors with the above syntax based on the currently defined theme.

This PR allows for these dynamic variables to be passed. It's my first PR, so feedback is welcome, especially on my RegEx 😅

cc @nicolaskruchten — I've tested this with px as well as plotly.graph_objs

@nicolaskruchten
Copy link
Contributor

I haven’t looked at the implementation but I think this is a good solution in principle to the problem of allowing PX and DDK to interoperate, even if it looks a bit weird. I’d be fine with putting this behind a flag that px could turn on if you think that’s necessary, @jonmmease but not doing that makes it easier to interop with DDK more broadly even outside of PX :)

@jonmmease
Copy link
Contributor

Thanks @wbrgss,

The regex looks fine so long as it covers everything DDK needs (which I'm not familiar enough to judge).

Just to help me better understand the workflow here, will users set these colors in their own code or are these implementation details for DDKs own internal use?

Are there cases where a user might specify a color this way thinking that they are using DDK, but accidentally not using DDK? Are there any conditions under which plotly.py could check to make sure that DDK is active before accepting these colors?

@jonmmease
Copy link
Contributor

BTW, don't worry about the plotlyjs_dev_build failure here. That's building plotly.py against plotly.js master and we just use that to build dev releases and as an early warning for problems that might be waiting for us on the next plotly.js release 🙂

@nicolaskruchten
Copy link
Contributor

The main idea is that ddk will set the default PX color scales to these magic variables up front, then px runs and sprinkles them throughout a figure, then ddk picks them up on the way out and substitutes them client side with the desired color scheme... is that about right @wbrgss ?

@nicolaskruchten
Copy link
Contributor

I’d say the odds are low that a non-ddk user would stumble across this and use them unknowingly

@wbrgss
Copy link
Contributor Author

wbrgss commented May 1, 2019

@jonmmease

The regex looks fine so long as it covers everything DDK needs (which I'm not familiar enough to judge).

Yeah, it's pretty specific. I left the wildcard between var(-- and ) because we will probably add more theme variables in the future, perhaps with syntax TBD (right now it's [A-Za-z0-9\-_.])

Just to help me better understand the workflow here, will users set these colors in their own code or are these implementation details for DDKs own internal use?

Users will set the colors in their own code to theme variables, which will be mapped by DDK in the front end to the theme. In their app, users can set a static theme{} dict, and use the keys from there, but then their graph colors wouldn't update dynamically when they change the theme on the front-end. This PR allows for that. @nicolaskruchten explained the desired flow with px well; we obviously want users to be able to use these variables with plotly.graph_objs on its own as well.

Are there cases where a user might specify a color this way thinking that they are using DDK, but accidentally not using DDK? Are there any conditions under which plotly.py could check to make sure that DDK is active before accepting these colors?

Great question — it's possible that a user might specify a color this way because it's the same as CSS syntax, and be confused as to why they can't set a trace color to a CSS variable. I agree with @nicolaskruchten that it's not very likely, however. This possibility could be mitigated by...

Are there any conditions under which plotly.py could check to make sure that DDK is active before accepting these colors?

Yeah, I thought about this as well. The figures generated by plotly.py can safely be assumed to be used in a ddk.Graph(), but of course they can be instantiated outside that scope. So the only semi-reliable way I can think of doing this is e.g.

import sys
if 'dash_design_kit' in sys.modules:
[...]

Does that make sense or does it seem like a hack to you guys?

@jonmmease
Copy link
Contributor

Thanks for the background @nicolaskruchten and @wbrgss,

I'm comfortable with merging this as is. We can look at validating that ddk is loaded if this trips people up in the future, but lets start without it for simplicity.

@jonmmease jonmmease merged commit 8162927 into master May 2, 2019
@wbrgss
Copy link
Contributor Author

wbrgss commented May 2, 2019

Thank you @jonmmease!

@wbrgss wbrgss deleted the ddk-compatibility branch May 2, 2019 17:09
@nicolaskruchten
Copy link
Contributor

(would also be great to have this guy be part of 3.9, released sooner than plotly.js 1.48 ... :D )

@jonmmease jonmmease added this to the v3.9.0 milestone May 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants