Skip to content

Date and number localization #2207

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.

Sign up for GitHub

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

Merged
alexcjohnson merged 11 commits into from
Dec 18, 2017
Merged

Date and number localization #2207

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
eacc99b
use d3.locale formats just like the localization dicts of #2195
alexcjohnson Dec 15, 2017
fe812eb
pull in all locales existing in world-calendars
alexcjohnson Dec 15, 2017
64968dd
move locales en and en-US into the core
alexcjohnson Dec 15, 2017
a527828
:see_no_evil:
alexcjohnson Dec 15, 2017
43b73e0
test case correctness of all require statements
alexcjohnson Dec 15, 2017
27037af
combine dictionaries and formats into locales
alexcjohnson Dec 15, 2017
2a8d38f
explicit test for #1264
alexcjohnson Dec 15, 2017
d0e0158
use require.resolve in pull_date_format
alexcjohnson Dec 15, 2017
592fc9e
:hocho: unused trace._separators
alexcjohnson Dec 15, 2017
614d5be
move lib/locale-* to lib/locales/* and update scripts accordingly
alexcjohnson Dec 18, 2017
331b2da
describe localization in dist/README.md
alexcjohnson Dec 18, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/core.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ exports.register([
// locales en and en-US are required for default behavior
exports.register([
require('./locale-en'),
require('./locale-en-US')
require('./locale-en-us')
Copy link
Collaborator Author

@alexcjohnson alexcjohnson Dec 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how did this not fail for me locally but only on CI?

Copy link
Collaborator Author

@alexcjohnson alexcjohnson Dec 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a check for case correctness of all the requires 43b73e0

]);
Copy link
Collaborator Author

@alexcjohnson alexcjohnson Dec 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎩 bundle_tests/core_test 🌟

Since we're constructing our own d3.locale now no matter what, locale-en needs to be part of the core so it can be used as the fallback for missing pieces of other locales. Then for simplicity I moved the default locale-en-US to the core as well, which has the nice side-effect of keeping these redundant locales out of dist/.

@etpinard seem reasonable? Are you OK with putting these two in the root src/ directory? At least as long as we're not going to put any other locales into the core it doesn't seem like they warrant their own directory.

Minor downside is the lib/index* files don't demonstrate how to register locales, but we should be able to to better than documenting it there anyhow - somewhere we will want examples of how to use locales both in the build process and via script tags.

Copy link
Contributor

@etpinard etpinard Dec 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@etpinard seem reasonable?

This is very reasonable 👌

but we should be able to to better than documenting it there anyhow

Yep, we should at least open up an issue on https://github.com/plotly/documentation/issues and / or add a Locale section to dist/README.md via tasks/stats.js.

Copy link
Collaborator Author

@alexcjohnson alexcjohnson Dec 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated dist/README.md -> 331b2da

will open an issue at https://github.com/plotly/documentation once this is merged.


// plot icons
Expand Down
2 changes: 1 addition & 1 deletion src/plots/cartesian/set_convert.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -447,7 +447,7 @@ module.exports = function setConvert(ax, fullLayout) {
ax._min = [];
ax._max = [];

// Fropagate localization into the axis so that
// Propagate localization into the axis so that
Copy link
Contributor

@etpinard etpinard Dec 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

// methods in Axes can use it w/o having to pass fullLayout
// Default (non-d3) number formatting uses separators directly
// dates and d3-formatted numbers use the d3 locale
Expand Down