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

On-figure error for mapbox-token-related problems #4098

Closed
nicolaskruchten opened this issue Aug 1, 2019 · 15 comments · Fixed by #4464
Closed

On-figure error for mapbox-token-related problems #4098

nicolaskruchten opened this issue Aug 1, 2019 · 15 comments · Fixed by #4464
Labels
feature something new
Milestone

Comments

@nicolaskruchten
Copy link
Contributor

nicolaskruchten commented Aug 1, 2019

For the two token-related errors, we should show the error in the figure:

  • no token, no style
  • mapbox style, no token

There's a third we could try to catch:

  • mapbox style, mapbox token: invalid token
@etpinard etpinard added the feature something new label Aug 1, 2019
@etpinard etpinard self-assigned this Aug 2, 2019
@etpinard etpinard added this to the v1.50.0 milestone Aug 2, 2019
@etpinard etpinard removed this from the v1.50.0 milestone Sep 25, 2019
@etpinard etpinard removed their assignment Sep 26, 2019
@nicolaskruchten nicolaskruchten added this to the v1.52.0 milestone Oct 31, 2019
@etpinard
Copy link
Contributor

etpinard commented Dec 24, 2019

(Writing down a few thoughts from a private convo with @nicolaskruchten )

Instead of showing the errors on the figure (like we currently do for WebGL-initialisation errors), perhaps it would be better to add a way to propagate ALL Lib.log, Lib.warn and Lib.error messages out of the JS console and into the graph div behind some new config flag (e.g. named notifyLogs). I'm thinking of reusing the Lib.notifier we currently use for e.g. double-click tips to show those messages.

@nicolaskruchten
Copy link
Contributor Author

This would be great for the “totals don’t match” warnings from sunburst/treemap as well for Python/R users who won’t naturally go check the console or might not be able to easily (ie in RStudio)

@alexcjohnson
Copy link
Collaborator

That sounds great, except that Lib.notifier goes away by itself fairly quickly. These errors should stick around until the user clears them. Hopefully a small tweak to make notifier support that mode.

@etpinard
Copy link
Contributor

Here's a WIP branch that's add a new config option that allows log/warn/error messages to show up in "notifier" on-graph popups

https://github.com/plotly/plotly.js/compare/onGraphLogging

Demos:

Let me know what you think.

@archmoj
Copy link
Contributor

archmoj commented Dec 31, 2019

Nicely done. I'd vote for reducing the transition time a bit.

@etpinard
Copy link
Contributor

etpinard commented Jan 6, 2020

Ping @nicolaskruchten

@nicolaskruchten
Copy link
Contributor Author

Looks awesome to me! I wonder about the name onGraphLogging though... it's not really on graph any more? how about something like notifyOnWarnings or something?

@etpinard
Copy link
Contributor

etpinard commented Jan 6, 2020

onGraphLogging comes from the current logging config option, but yeah we could do better. I'd be ok with notifyOnWarnings if you prefer.

@nicolaskruchten
Copy link
Contributor Author

Semi-related: what is the current "level" for hierarchy-related issues in sunburst and treemap? Right now they don't show up in the Dash "debug" mode developer tools because they're not JS errors being thrown I think. The Mapbox-token-related stuff does show up though. It would be really helpful for Dash users to have the sunburst/treemap stuff appear in the Dash dev tools.

image

@etpinard
Copy link
Contributor

etpinard commented Jan 6, 2020

what is the current "level" for hierarchy-related issues in sunburst and treemap?

They use Lib.warn, so they should show up in the console under logging: 1 or logging: 2.

@etpinard
Copy link
Contributor

etpinard commented Jan 6, 2020

Maybe dash only catches the throw new Error() ?

@nicolaskruchten
Copy link
Contributor Author

Yeah it does. Is there a way to catch the log messages from a wrapper?

@nicolaskruchten
Copy link
Contributor Author

Or could we consider making the hierarchy stuff actual errors? they seem to be at the same level of severity as mapbox token stuff to me...

@etpinard
Copy link
Contributor

etpinard commented Jan 6, 2020

Or could we consider making the hierarchy stuff actual errors?

We could, but to me mapbox here is the exception.

At present, we don't throw errors on bad data/layout inputs. We only throw errors on bad API calls (e.g. when you call Plotly.newPlot(gd) with gd not being a <div>) and on bad config input (e.g. when setting modeBarButtonsToAdd) but never on data/layout inputs. Now mapbox access tokens can be set as a config option OR a layout attribute, so there's a bit of an inconsistency there, but still.

Yeah it does. Is there a way to catch the log messages from a wrapper?

Not that I can think of, but it might be cool to trigger events e.g. plotly_log, plotly_warn, plotly_error.

@nicolaskruchten
Copy link
Contributor Author

Works for me: #4462

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature something new
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants