plotly / plotly.js

Open-source JavaScript charting library behind Plotly and Dash
https://plotly.com/javascript/
MIT License
17.14k stars 1.87k forks source link

"dash length list in px" option not specified in schema #2903

Closed jonmmease closed 5 months ago

jonmmease commented 6 years ago

See https://github.com/plotly/plotly.py/issues/1107.

Plotly.py version 3 is not allowing a scatter.line.dash property to be set to a list of lengths in pixels (e.g. '5px,10px,2px,2px'). The issue is that the schema lists the property as an enumeration, and doesn't indicate this pixel list form:

                    "dash": {
                        "valType": "string",
                        "values": [
                            "solid",
                            "dot",
                            "dash",
                            "longdash",
                            "dashdot",
                            "longdashdot"
                        ],
                        "dflt": "solid",
                        "role": "style",
                        "editType": "style",
                        "description": "Sets the dash style of lines. Set to a dash type string (*solid*, *dot*, *dash*, *longdash*, *dashdot*, or *longdashdot*) or a dash length list in px (eg *5px,10px,2px,2px*)."
                    },

How would you recommend proceeding in plotly.py? Should dashes be a special case (which is fine on my end), or is there something that we should do in the schema?

Thanks!

jonmmease commented 6 years ago

Thinking more about it, could we add a regular expression to the values list that would handle this case?

alexcjohnson commented 6 years ago

Right, we have valType: 'string' so internally we don't actually use the values at all. The code has this comment:

    // string type usually doesn't take values... this one should really be
    // a special type or at least a special coercion function, from the GUI
    // you only get these values but elsewhere the user can supply a list of
    // dash lengths in px, and it will be honored

For now I'd make this a special case, but @etpinard what about folding this into enumerated and adding a field like

regExpValues: [/^(\d+(\.\d+)?px,)*\d+(\.\d+)?px$/]

ie in principle a list of RegExp (but we only need one here) that during PlotSchema.get() we convert into a list of strings

regExpValues: ['^(\d+(\.\d+)?px,)*\d+(\.\d+)?px$']

(which is slightly more complicated than just .toString() because that leaves the / bookends that other languages won't want... but that's easy to fix.)

Oddly, although we've always used px as part of the string, it's really not supposed to be there and seems to just be ignored by all the browsers we support. So I guess we should continue to support it but make it optional and adapt the docs to discourage it. And you can use commas or whitespace between items. And then there's also an option to specify percentages, that I can see occasionally being convenient... https://www.w3.org/TR/SVG/painting.html#StrokeDashing

So the ideal regexp might be: /^(\d+(\.\d+)?(px|%)?(,|\s))*\d+(\.\d+)?(px|%)?$/

etpinard commented 6 years ago

Yep, regExpValues sounds like a good way forward.

About toString, it should be too hard to modify

https://github.com/plotly/plotly.js/blob/def6aa5a24527c68e90f50485255170609640e43/src/plot_api/plot_schema.js#L656-L659

to output what we want in the schema json.

jonmmease commented 6 years ago

Thanks @alexcjohnson and @etpinard,

I'll make it a special case and use @alexcjohnson 's regular expression as a starting point for our own validation. Even if we do add the regular expression to the schema, a special case is probably more user-friendly than telling a user that their string must match this regular expression 🙂

Here's the full list of traces that have a line.dash property.

screen shot 2018-08-15 at 6 21 37 am

Based on the descriptions, it looks like all of them except scatterpolargl, scattergl, and scatter3d support the custom length dashes. Does that sound right?

etpinard commented 6 years ago

all of them except scatterpolargl, scattergl, and scatter3d support the custom length dashes. Does that sound right?

That's correct :+1:

gvwilson commented 5 months ago

Hi - this issue has been sitting for a while, so as part of our effort to tidy up our public repositories I'm going to close it. If it's still a concern, we'd be grateful if you could open a new issue (with a short reproducible example if appropriate) so that we can add it to our stack. Cheers - @gvwilson