Skip to content

Conversation

@nicolaskruchten
Copy link
Contributor

@nicolaskruchten nicolaskruchten commented Dec 12, 2018

Known RCE issues:

  • text editor reads container[attr] and doesn't work if attr contains dots
  • colorscale horizontal alignment
  • is2D and array adjustments in DataSelector
  • hide hovertemplate when empty
  • wait for plotly.js to stop writebacks of colorscale to marker so that the layout.colorscales actually control things

@nicolaskruchten
Copy link
Contributor Author

@VeraZab if you're willing to dive back into the colorscale pickers there's a bundle of issues here although if you're sick of them maybe it's @dmt0's turn for a tour of duty in there, and you can take the others ;)

@nicolaskruchten
Copy link
Contributor Author

Let’s ignore Percy for now, I think the test failures are due to the way I pointed package.json to master. Should go away when 1.43.0 actually comes out

@nicolaskruchten
Copy link
Contributor Author

image

@dmt0
Copy link
Contributor

dmt0 commented Dec 14, 2018

Hover template really needs to show a list of available variables and whatever formatting options are there.

@nicolaskruchten
Copy link
Contributor Author

Yeah we need something more discoverable for hover templates, and in a few other places with "magic formatting text". I'm fine with just making it available for now and waiting for the documentation page to be added so we can link to it or something like that. We could/should also consider making a flyout/tooltip/info component for long help text

@VeraZab
Copy link
Contributor

VeraZab commented Dec 17, 2018

Hey, I see that in the panelTest.json, that Percy uses I think?, there's references to titlefont
We should change this to title.font
Also, it's in test-utils.js

@VeraZab
Copy link
Contributor

VeraZab commented Dec 17, 2018

In streambed, I think that we should set that watermark config option to false.. it's a bit annoying to see that plotly logo on the plot and on the header of the EditModeMenu no?

@VeraZab
Copy link
Contributor

VeraZab commented Dec 17, 2018

I think we should find a way to make this be centered
screen shot 2018-12-17 at 11 22 24 am

@nicolaskruchten
Copy link
Contributor Author

The watermark config option is false by default... You want to hide the logo in streambed? It's been there forever AFAIK...

@nicolaskruchten
Copy link
Contributor Author

Re centeredness ... I don't think this is going to be possible, as we'd have to rejig the label/content heirarchy. I remember you looked at this for a while when we redid it the first time :)

@VeraZab
Copy link
Contributor

VeraZab commented Dec 17, 2018

well, I went through the api changes and code and played around with this a little locally, it seems good. Except I think a little more attention should be given to colorscales.. and we should review the json example figures to see if there's anything more to add or adjust there to take into account the new api additions..

I think worst case, for colorscales, we could maybe remove the dropdown for the different colorscale types, as the title says that the colorscales presented to us should be of a specific type?

@VeraZab
Copy link
Contributor

VeraZab commented Dec 17, 2018

screen shot 2018-12-17 at 11 29 19 am

My labels and dividers are superposed, I'm not sure which option would enable me to adjust that.
And I guess I can't style the levels of axes labels individually... make first level labels smaller

Ah, I see, well with rotation it's better
screen shot 2018-12-17 at 11 33 07 am

Was just wondering if we had exposed everything related to those dividers positioning

@nicolaskruchten
Copy link
Contributor Author

that divider-positioning thing seems like a plotly.js bug, or you reset tickson to be 'labels' ? by default it's 'boundaries' for multicat, which is how this is avoided.

@nicolaskruchten
Copy link
Contributor Author

a little more attention should be given to colorscales

say more?

@VeraZab
Copy link
Contributor

VeraZab commented Dec 17, 2018

The watermark config option is false by default... You want to hide the logo in streambed? It's been there forever AFAIK...

I think it just recently got restyled or something, it's a lot more visible than I remember it to be.. I don't know, just felt like overbranding. But I guess that maybe it should stay, because then that plot is saved, and wherever its used, if people don't mind it being there, its good branding for us.. Just thought it looked more visible than I remember it

@VeraZab
Copy link
Contributor

VeraZab commented Dec 17, 2018

a little more attention should be given to colorscales

well, maybe this:

we could maybe remove the dropdown for the different colorscale types, as the title says that the colorscales presented to us should be of a specific type

Would make it look less jammed, since the colorscale category is in the label?

This section is more jammed than its currently on prod:
screen shot 2018-12-17 at 11 38 37 am

@nicolaskruchten
Copy link
Contributor Author

The logo was updated to match our current branding a couple of versions ago I think, and yeah, the color is punchier and no longer matches the modebar buttons so it's more visible. I think we'll keep it for now.

The watermark feature is much more invasive: when turned on, the logo is always there/doesn't fade away with the modebar. We'll likely turn it on for Community-plan embeds only.

Re colorscale pickers, I agree that the dropdown feels a bit redundant here, but people do like using sequential scales instead of categoricals even though they really shouldn't etc, so let's just keep them for now :)

@nicolaskruchten
Copy link
Contributor Author

Can you show side-by-side what's more jammed now than in prod?

@VeraZab
Copy link
Contributor

VeraZab commented Dec 17, 2018

screen shot 2018-12-17 at 11 48 12 am

@nicolaskruchten
Copy link
Contributor Author

Ah I see. that's probably related to @dmt0's fix for alignment. although arguably there's less spacing but the scales themselves have more room. I'm OK with the current state of affairs but if we can easily add some spacing I'd be fine too.

@nicolaskruchten nicolaskruchten changed the title WIP Pre143 Plotly.js 1.43.0 upgrade Dec 19, 2018
@dmt0
Copy link
Contributor

dmt0 commented Dec 19, 2018

💃

@nicolaskruchten nicolaskruchten merged commit eb79dab into master Dec 19, 2018
@VeraZab VeraZab deleted the pre143 branch December 19, 2018 20:35
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.

4 participants