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

Upgrade d3 formats #5023

Closed
wants to merge 6 commits into from
Closed

Upgrade d3 formats #5023

wants to merge 6 commits into from

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Jul 27, 2020

Attempt using latest d3-format and d3-time-format to resolve #5009 | Demo.

Notes:

@plotly/plotly_js

@nicolaskruchten
Copy link
Contributor

Is it necessary to update d3-format as well as d3-time-format or can we just do the time update?

Certain formats e.g. 0.f are no longer supported by d3 and throw an error.

This seems like a showstopper.

@alexcjohnson
Copy link
Collaborator

Certain formats e.g. 0.f are no longer supported by d3 and throw an error.

There are two issues with this:

  • We shouldn't ever throw an error, we should fall back on some default formatting and log an error (probably just once, otherwise this could easily dump thousands of logs)
  • If possible we shouldn't lose any behavior that it's possible to keep.

Date formatting all goes through lib.formatDate - if there are cases where date formatting throws an error, we should be able to catch these there and convert them to errors. (@archmoj is that true? Any date formatting that we DON'T pass through lib.formatDate? If so, can we?)

Perhaps then we need a similar wrapper for number formatting to catch errors, and then we can use this wrapper to backfill removed functionality? It's not actually clear to me from the D3 changelog whether this is actually removed functionality, or perhaps it was previously falling back on a default but now throws an error instead?

If it's going to take a long time to be confident we're not breaking important behavior then sure, @nicolaskruchten's suggestion to upgrade only time is reasonable. But if it can be done quickly seems worth pressing ahead.

@archmoj
Copy link
Contributor Author

archmoj commented Jul 28, 2020

Is it necessary to update d3-format as well as d3-time-format or can we just do the time update?

It seems necessary as they both depend on d3 locale.
So we'll need to make a similar wrapper for number formatting as suggested by Alex above.

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 this pull request may close these issues.

Formatting for weeks and quarters
3 participants