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

Pr/legend locale #71

Merged
merged 6 commits into from
Nov 17, 2017
Merged

Pr/legend locale #71

merged 6 commits into from
Nov 17, 2017

Conversation

existe-deja
Copy link

You can see the tests to understand how it works.
The basic idea:

  1. Create a locale object in the legend
  2. Adapt labelFormat to use the locale's format. To do it I choose to use a specifier instead of the format function
  3. Be sure to support the two ways to set labelFormat, by string or by d3.format

Let me know what to change. When it will be ok for you, I'll do the implementation for sizeLegend and symbolLegend too.

src/legend.js Outdated
thousands: ',',
grouping: [3],
currency: ['$', '']
},
Copy link
Owner

Choose a reason for hiding this comment

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

It looks like in the d3-format module it is published with the locale folder, so for this we can import something like d3-format/local/en-US.json and use this as the default definition instead of remaking it here.

Copy link
Author

Choose a reason for hiding this comment

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

I agree, this is not the most efficiant way to get the locale. But I'm facing two problems:

  1. I cannot access to the default locale, calling d3.formatLocale() without args throws an error
  2. I cannot import the locales as simple as you said because of Publish locales as JSON data, not code. d3/d3-format#22 and External access to locale definitions? d3/d3-format#8

Should I import like this?
require('../node_modules/d3-format/locale/en-US.json')
I'm not fan

Copy link
Author

Choose a reason for hiding this comment

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

I found an another way, a locale is no more than an object with a format and formatPrefix methods, so I created a default one with the package methods.

@susielu
Copy link
Owner

susielu commented Nov 14, 2017

Looks good, I just had one comment about the default format. Let me know when it's in the other two legend types and I'll take a look locally and update the docs. Thanks!

@susielu
Copy link
Owner

susielu commented Nov 15, 2017

This looks good to me, tested it out on the docs and it worked well. Did you have any other changes you wanted to make?

@existe-deja
Copy link
Author

I think we're good! It's on your hands now to build and update.
By the way, it was my first open source contribution and I was very happy to do it.

@susielu
Copy link
Owner

susielu commented Nov 16, 2017

@LaCroute exciting, congrats! It was great having your help to implement this feature. I'll move it in today and update the docs. Cheers!

@susielu susielu merged commit 822148f into susielu:master Nov 17, 2017
@existe-deja existe-deja deleted the pr/legend-locale branch November 17, 2017 08:54
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.

2 participants