Localize auto-formatted x-axis date ticks

[TomDemulierChevret]

Features, Bug fixes, and others:

This pull request add the possibility to localize the auto-formatted x-axis date ticks.
It countains a working example for the french locale.

If some (or all) of the new formats are missing in the locale set by the user, format from the en locale will be used (as it does for the base d3 format and for the translation keys).

[etpinard]

Thanks for the PR. Looks like @alexcjohnson was planning on doing this, but left it as a TODO.

A few tests are failing. I suspect formatDate is getting called outside of formatWorld w/o the extraFormat argument causing it to break.

[alexcjohnson]

modDateFormat calls out to components/calendars to get the full spectrum of d3-to-world-cal conversions - you've got the ones needed so far, but others will likely show up, and no need to reinvent the wheel with formatWorld.

At one point there was a performance argument for the structure we have here because we could use precompiled formatters... but we lost that benefit with the original date localization PR #2207, so now I think there would be a much more concise way to do this, something like:

calendar=isWorldCalendar(calendar)&&calendar;if(!fmt){if(tr==='y')fmt=extraFormat.year;elseif(tr==='m')fmt=extraFormat.month;elseif(tr==='d'){fmt=extraFormat.dayMonth+'\n'+extraFormat.year;}else{returnmodDateFormat(extraFormat.dayMonthYear,x,formatter,calendar)+'\n'+formatTime(x,tr);}}returnmodDateFormat(fmt,x,formatter,calendar);

[TomDemulierChevret]

Yeah, I suspected that this conversion table existed somewhere but I didn't know exactly where to look (and code search with date/format/etc returned way too many entries).

I like the new proposal, pretty clear to read.

[TomDemulierChevret]

Just test and it does work indeed on my example (only the last else is incorrect, need to switch date and time).

[alexcjohnson]

%e is padded with a space for 1-digit days - we should switch back to %-d, which has no padding (in fr.js as well). That's responsible for the image test failures.

[alexcjohnson]

@TomDemulierChevret are you able to get the tests to run locally? It looks like axes_test and lib_date_test are both failing, @etpinard probably had the right idea there. We should also add a couple of explicit tests in localize_test to cover the newly added fields.

You can run just these three suites with the command:
npm run test-jasmine -- axes lib_date localize

[TomDemulierChevret]

Well I had a problem on my first try, but I will try again tomorrow.

[TomDemulierChevret]

Sorry, didn't have time to check this issue on Friday.

Regarding axes_test, the issue was with the initialisation of ax in setConvert method.
The fullLayout parameter passed to it has always an _extraFormat member from now on (retrieved from the locale) but since it's manually created in the test, this member was missing.

Regarding lib_date_test, the issue was that method formatDate was called directly without the new extraFormat parameter (which is normally passed via ax like in the axes_test).
I looked throught the code and couldn't find an occurence of formatDate called in another place than axes.js (which provides the extraFormat paramater as stated before).
Therefore I added manually the extraFormat to formatDate calls in lib_date_test when needed.

But since my knowledge of plotly.js is pretty limited, I may have missed some usage of formatDate, so you should definitly check there isn't a broken call somewhere.

I will try to add new test in localize_test to cover the new fields.

[TomDemulierChevret]

Just added a new test which check that both default locale (en) & provided locale format correctly the auto-formatted x-axis date tick.

Do you feel it is enough or should I add more test case ?

If everything is okay, can this PR be added to the next release ?

[alexcjohnson]

@TomDemulierChevret looks great - thanks for the fixes, and the new test covers it all nicely.

I'm re-running the tests - we've had a lot of spurious failures lately just when running the tests on CI, we're working on making those more robust - assuming it passes (eventually) I think this is ready to go! 💃

I'm going to wait a few days before merging, in case we need a patch release since we just put out several major new features. But yes, this will be in 1.34.0 🎉

[TomDemulierChevret]

Perfect !
I'm not really in a hurry to have this merged, it was just to know that it's planned.
I'm gonna use plotly in an upcomming project in which locale support is mandatory, given it seems to be the best JS chart/plot library it would've been a shame to not use it because of locale. ;)

[TomDemulierChevret]

Features, Bug fixes, and others:

This pull request add the possibility to localize the auto-formatted x-axis date ticks.
It countains a working example for the french locale.

If some (or all) of the new formats are missing in the locale set by the user, format from the en locale will be used (as it does for the base d3 format and for the translation keys).

[etpinard]

Thanks for the PR. Looks like @alexcjohnson was planning on doing this, but left it as a TODO.

A few tests are failing. I suspect formatDate is getting called outside of formatWorld w/o the extraFormat argument causing it to break.

[alexcjohnson]

modDateFormat calls out to components/calendars to get the full spectrum of d3-to-world-cal conversions - you've got the ones needed so far, but others will likely show up, and no need to reinvent the wheel with formatWorld.

At one point there was a performance argument for the structure we have here because we could use precompiled formatters... but we lost that benefit with the original date localization PR #2207, so now I think there would be a much more concise way to do this, something like:

calendar=isWorldCalendar(calendar)&&calendar;if(!fmt){if(tr==='y')fmt=extraFormat.year;elseif(tr==='m')fmt=extraFormat.month;elseif(tr==='d'){fmt=extraFormat.dayMonth+'\n'+extraFormat.year;}else{returnmodDateFormat(extraFormat.dayMonthYear,x,formatter,calendar)+'\n'+formatTime(x,tr);}}returnmodDateFormat(fmt,x,formatter,calendar);

[TomDemulierChevret]

Yeah, I suspected that this conversion table existed somewhere but I didn't know exactly where to look (and code search with date/format/etc returned way too many entries).

I like the new proposal, pretty clear to read.

[TomDemulierChevret]

Just test and it does work indeed on my example (only the last else is incorrect, need to switch date and time).

[alexcjohnson]

%e is padded with a space for 1-digit days - we should switch back to %-d, which has no padding (in fr.js as well). That's responsible for the image test failures.

[alexcjohnson]

@TomDemulierChevret are you able to get the tests to run locally? It looks like axes_test and lib_date_test are both failing, @etpinard probably had the right idea there. We should also add a couple of explicit tests in localize_test to cover the newly added fields.

You can run just these three suites with the command:
npm run test-jasmine -- axes lib_date localize

[TomDemulierChevret]

Well I had a problem on my first try, but I will try again tomorrow.

[TomDemulierChevret]

Sorry, didn't have time to check this issue on Friday.

Regarding axes_test, the issue was with the initialisation of ax in setConvert method.
The fullLayout parameter passed to it has always an _extraFormat member from now on (retrieved from the locale) but since it's manually created in the test, this member was missing.

Regarding lib_date_test, the issue was that method formatDate was called directly without the new extraFormat parameter (which is normally passed via ax like in the axes_test).
I looked throught the code and couldn't find an occurence of formatDate called in another place than axes.js (which provides the extraFormat paramater as stated before).
Therefore I added manually the extraFormat to formatDate calls in lib_date_test when needed.

But since my knowledge of plotly.js is pretty limited, I may have missed some usage of formatDate, so you should definitly check there isn't a broken call somewhere.

I will try to add new test in localize_test to cover the new fields.

[TomDemulierChevret]

Just added a new test which check that both default locale (en) & provided locale format correctly the auto-formatted x-axis date tick.

Do you feel it is enough or should I add more test case ?

If everything is okay, can this PR be added to the next release ?

[alexcjohnson]

@TomDemulierChevret looks great - thanks for the fixes, and the new test covers it all nicely.

I'm re-running the tests - we've had a lot of spurious failures lately just when running the tests on CI, we're working on making those more robust - assuming it passes (eventually) I think this is ready to go! 💃

I'm going to wait a few days before merging, in case we need a patch release since we just put out several major new features. But yes, this will be in 1.34.0 🎉

[TomDemulierChevret]

Perfect !
I'm not really in a hurry to have this merged, it was just to know that it's planned.
I'm gonna use plotly in an upcomming project in which locale support is mandatory, given it seems to be the best JS chart/plot library it would've been a shame to not use it because of locale. ;)

[TomDemulierChevret]

Features, Bug fixes, and others:

This pull request add the possibility to localize the auto-formatted x-axis date ticks.
It countains a working example for the french locale.

If some (or all) of the new formats are missing in the locale set by the user, format from the en locale will be used (as it does for the base d3 format and for the translation keys).

[etpinard]

Thanks for the PR. Looks like @alexcjohnson was planning on doing this, but left it as a TODO.

A few tests are failing. I suspect formatDate is getting called outside of formatWorld w/o the extraFormat argument causing it to break.

[alexcjohnson]

modDateFormat calls out to components/calendars to get the full spectrum of d3-to-world-cal conversions - you've got the ones needed so far, but others will likely show up, and no need to reinvent the wheel with formatWorld.

At one point there was a performance argument for the structure we have here because we could use precompiled formatters... but we lost that benefit with the original date localization PR #2207, so now I think there would be a much more concise way to do this, something like:

calendar=isWorldCalendar(calendar)&&calendar;if(!fmt){if(tr==='y')fmt=extraFormat.year;elseif(tr==='m')fmt=extraFormat.month;elseif(tr==='d'){fmt=extraFormat.dayMonth+'\n'+extraFormat.year;}else{returnmodDateFormat(extraFormat.dayMonthYear,x,formatter,calendar)+'\n'+formatTime(x,tr);}}returnmodDateFormat(fmt,x,formatter,calendar);

[TomDemulierChevret]

Yeah, I suspected that this conversion table existed somewhere but I didn't know exactly where to look (and code search with date/format/etc returned way too many entries).

I like the new proposal, pretty clear to read.

[TomDemulierChevret]

Just test and it does work indeed on my example (only the last else is incorrect, need to switch date and time).

[alexcjohnson]

%e is padded with a space for 1-digit days - we should switch back to %-d, which has no padding (in fr.js as well). That's responsible for the image test failures.

[alexcjohnson]

@TomDemulierChevret are you able to get the tests to run locally? It looks like axes_test and lib_date_test are both failing, @etpinard probably had the right idea there. We should also add a couple of explicit tests in localize_test to cover the newly added fields.

You can run just these three suites with the command:
npm run test-jasmine -- axes lib_date localize

[TomDemulierChevret]

Well I had a problem on my first try, but I will try again tomorrow.

[TomDemulierChevret]

Sorry, didn't have time to check this issue on Friday.

Regarding axes_test, the issue was with the initialisation of ax in setConvert method.
The fullLayout parameter passed to it has always an _extraFormat member from now on (retrieved from the locale) but since it's manually created in the test, this member was missing.

Regarding lib_date_test, the issue was that method formatDate was called directly without the new extraFormat parameter (which is normally passed via ax like in the axes_test).
I looked throught the code and couldn't find an occurence of formatDate called in another place than axes.js (which provides the extraFormat paramater as stated before).
Therefore I added manually the extraFormat to formatDate calls in lib_date_test when needed.

But since my knowledge of plotly.js is pretty limited, I may have missed some usage of formatDate, so you should definitly check there isn't a broken call somewhere.

I will try to add new test in localize_test to cover the new fields.

[TomDemulierChevret]

Just added a new test which check that both default locale (en) & provided locale format correctly the auto-formatted x-axis date tick.

Do you feel it is enough or should I add more test case ?

If everything is okay, can this PR be added to the next release ?

[alexcjohnson]

@TomDemulierChevret looks great - thanks for the fixes, and the new test covers it all nicely.

I'm re-running the tests - we've had a lot of spurious failures lately just when running the tests on CI, we're working on making those more robust - assuming it passes (eventually) I think this is ready to go! 💃

I'm going to wait a few days before merging, in case we need a patch release since we just put out several major new features. But yes, this will be in 1.34.0 🎉

[TomDemulierChevret]

Perfect !
I'm not really in a hurry to have this merged, it was just to know that it's planned.
I'm gonna use plotly in an upcomming project in which locale support is mandatory, given it seems to be the best JS chart/plot library it would've been a shame to not use it because of locale. ;)

[TomDemulierChevret]

Features, Bug fixes, and others:

This pull request add the possibility to localize the auto-formatted x-axis date ticks.
It countains a working example for the french locale.

If some (or all) of the new formats are missing in the locale set by the user, format from the en locale will be used (as it does for the base d3 format and for the translation keys).

[etpinard]

Thanks for the PR. Looks like @alexcjohnson was planning on doing this, but left it as a TODO.

A few tests are failing. I suspect formatDate is getting called outside of formatWorld w/o the extraFormat argument causing it to break.

[alexcjohnson]

modDateFormat calls out to components/calendars to get the full spectrum of d3-to-world-cal conversions - you've got the ones needed so far, but others will likely show up, and no need to reinvent the wheel with formatWorld.

At one point there was a performance argument for the structure we have here because we could use precompiled formatters... but we lost that benefit with the original date localization PR #2207, so now I think there would be a much more concise way to do this, something like:

calendar=isWorldCalendar(calendar)&&calendar;if(!fmt){if(tr==='y')fmt=extraFormat.year;elseif(tr==='m')fmt=extraFormat.month;elseif(tr==='d'){fmt=extraFormat.dayMonth+'\n'+extraFormat.year;}else{returnmodDateFormat(extraFormat.dayMonthYear,x,formatter,calendar)+'\n'+formatTime(x,tr);}}returnmodDateFormat(fmt,x,formatter,calendar);

[TomDemulierChevret]

Yeah, I suspected that this conversion table existed somewhere but I didn't know exactly where to look (and code search with date/format/etc returned way too many entries).

I like the new proposal, pretty clear to read.

[TomDemulierChevret]

Just test and it does work indeed on my example (only the last else is incorrect, need to switch date and time).

[alexcjohnson]

%e is padded with a space for 1-digit days - we should switch back to %-d, which has no padding (in fr.js as well). That's responsible for the image test failures.

[alexcjohnson]

@TomDemulierChevret are you able to get the tests to run locally? It looks like axes_test and lib_date_test are both failing, @etpinard probably had the right idea there. We should also add a couple of explicit tests in localize_test to cover the newly added fields.

You can run just these three suites with the command:
npm run test-jasmine -- axes lib_date localize

[TomDemulierChevret]

Well I had a problem on my first try, but I will try again tomorrow.

[TomDemulierChevret]

Sorry, didn't have time to check this issue on Friday.

Regarding axes_test, the issue was with the initialisation of ax in setConvert method.
The fullLayout parameter passed to it has always an _extraFormat member from now on (retrieved from the locale) but since it's manually created in the test, this member was missing.

Regarding lib_date_test, the issue was that method formatDate was called directly without the new extraFormat parameter (which is normally passed via ax like in the axes_test).
I looked throught the code and couldn't find an occurence of formatDate called in another place than axes.js (which provides the extraFormat paramater as stated before).
Therefore I added manually the extraFormat to formatDate calls in lib_date_test when needed.

But since my knowledge of plotly.js is pretty limited, I may have missed some usage of formatDate, so you should definitly check there isn't a broken call somewhere.

I will try to add new test in localize_test to cover the new fields.

[TomDemulierChevret]

Just added a new test which check that both default locale (en) & provided locale format correctly the auto-formatted x-axis date tick.

Do you feel it is enough or should I add more test case ?

If everything is okay, can this PR be added to the next release ?

[alexcjohnson]

@TomDemulierChevret looks great - thanks for the fixes, and the new test covers it all nicely.

I'm re-running the tests - we've had a lot of spurious failures lately just when running the tests on CI, we're working on making those more robust - assuming it passes (eventually) I think this is ready to go! 💃

I'm going to wait a few days before merging, in case we need a patch release since we just put out several major new features. But yes, this will be in 1.34.0 🎉

[TomDemulierChevret]

Perfect !
I'm not really in a hurry to have this merged, it was just to know that it's planned.
I'm gonna use plotly in an upcomming project in which locale support is mandatory, given it seems to be the best JS chart/plot library it would've been a shame to not use it because of locale. ;)

[TomDemulierChevret]

Features, Bug fixes, and others:

This pull request add the possibility to localize the auto-formatted x-axis date ticks.
It countains a working example for the french locale.

If some (or all) of the new formats are missing in the locale set by the user, format from the en locale will be used (as it does for the base d3 format and for the translation keys).

[etpinard]

Thanks for the PR. Looks like @alexcjohnson was planning on doing this, but left it as a TODO.

A few tests are failing. I suspect formatDate is getting called outside of formatWorld w/o the extraFormat argument causing it to break.

[alexcjohnson]

modDateFormat calls out to components/calendars to get the full spectrum of d3-to-world-cal conversions - you've got the ones needed so far, but others will likely show up, and no need to reinvent the wheel with formatWorld.

At one point there was a performance argument for the structure we have here because we could use precompiled formatters... but we lost that benefit with the original date localization PR #2207, so now I think there would be a much more concise way to do this, something like:

calendar=isWorldCalendar(calendar)&&calendar;if(!fmt){if(tr==='y')fmt=extraFormat.year;elseif(tr==='m')fmt=extraFormat.month;elseif(tr==='d'){fmt=extraFormat.dayMonth+'\n'+extraFormat.year;}else{returnmodDateFormat(extraFormat.dayMonthYear,x,formatter,calendar)+'\n'+formatTime(x,tr);}}returnmodDateFormat(fmt,x,formatter,calendar);

[TomDemulierChevret]

Yeah, I suspected that this conversion table existed somewhere but I didn't know exactly where to look (and code search with date/format/etc returned way too many entries).

I like the new proposal, pretty clear to read.

[TomDemulierChevret]

Just test and it does work indeed on my example (only the last else is incorrect, need to switch date and time).

[alexcjohnson]

%e is padded with a space for 1-digit days - we should switch back to %-d, which has no padding (in fr.js as well). That's responsible for the image test failures.

[alexcjohnson]

@TomDemulierChevret are you able to get the tests to run locally? It looks like axes_test and lib_date_test are both failing, @etpinard probably had the right idea there. We should also add a couple of explicit tests in localize_test to cover the newly added fields.

You can run just these three suites with the command:
npm run test-jasmine -- axes lib_date localize

[TomDemulierChevret]

Well I had a problem on my first try, but I will try again tomorrow.

[TomDemulierChevret]

Sorry, didn't have time to check this issue on Friday.

Regarding axes_test, the issue was with the initialisation of ax in setConvert method.
The fullLayout parameter passed to it has always an _extraFormat member from now on (retrieved from the locale) but since it's manually created in the test, this member was missing.

Regarding lib_date_test, the issue was that method formatDate was called directly without the new extraFormat parameter (which is normally passed via ax like in the axes_test).
I looked throught the code and couldn't find an occurence of formatDate called in another place than axes.js (which provides the extraFormat paramater as stated before).
Therefore I added manually the extraFormat to formatDate calls in lib_date_test when needed.

But since my knowledge of plotly.js is pretty limited, I may have missed some usage of formatDate, so you should definitly check there isn't a broken call somewhere.

I will try to add new test in localize_test to cover the new fields.

[TomDemulierChevret]

Just added a new test which check that both default locale (en) & provided locale format correctly the auto-formatted x-axis date tick.

Do you feel it is enough or should I add more test case ?

If everything is okay, can this PR be added to the next release ?

[alexcjohnson]

@TomDemulierChevret looks great - thanks for the fixes, and the new test covers it all nicely.

I'm re-running the tests - we've had a lot of spurious failures lately just when running the tests on CI, we're working on making those more robust - assuming it passes (eventually) I think this is ready to go! 💃

I'm going to wait a few days before merging, in case we need a patch release since we just put out several major new features. But yes, this will be in 1.34.0 🎉

[TomDemulierChevret]

Perfect !
I'm not really in a hurry to have this merged, it was just to know that it's planned.
I'm gonna use plotly in an upcomming project in which locale support is mandatory, given it seems to be the best JS chart/plot library it would've been a shame to not use it because of locale. ;)

[TomDemulierChevret]

Features, Bug fixes, and others:

This pull request add the possibility to localize the auto-formatted x-axis date ticks.
It countains a working example for the french locale.

If some (or all) of the new formats are missing in the locale set by the user, format from the en locale will be used (as it does for the base d3 format and for the translation keys).

[etpinard]

Thanks for the PR. Looks like @alexcjohnson was planning on doing this, but left it as a TODO.

A few tests are failing. I suspect formatDate is getting called outside of formatWorld w/o the extraFormat argument causing it to break.

[alexcjohnson]

modDateFormat calls out to components/calendars to get the full spectrum of d3-to-world-cal conversions - you've got the ones needed so far, but others will likely show up, and no need to reinvent the wheel with formatWorld.

At one point there was a performance argument for the structure we have here because we could use precompiled formatters... but we lost that benefit with the original date localization PR #2207, so now I think there would be a much more concise way to do this, something like:

calendar=isWorldCalendar(calendar)&&calendar;if(!fmt){if(tr==='y')fmt=extraFormat.year;elseif(tr==='m')fmt=extraFormat.month;elseif(tr==='d'){fmt=extraFormat.dayMonth+'\n'+extraFormat.year;}else{returnmodDateFormat(extraFormat.dayMonthYear,x,formatter,calendar)+'\n'+formatTime(x,tr);}}returnmodDateFormat(fmt,x,formatter,calendar);

[TomDemulierChevret]

Yeah, I suspected that this conversion table existed somewhere but I didn't know exactly where to look (and code search with date/format/etc returned way too many entries).

I like the new proposal, pretty clear to read.

[TomDemulierChevret]

Just test and it does work indeed on my example (only the last else is incorrect, need to switch date and time).

[alexcjohnson]

%e is padded with a space for 1-digit days - we should switch back to %-d, which has no padding (in fr.js as well). That's responsible for the image test failures.

[alexcjohnson]

@TomDemulierChevret are you able to get the tests to run locally? It looks like axes_test and lib_date_test are both failing, @etpinard probably had the right idea there. We should also add a couple of explicit tests in localize_test to cover the newly added fields.

You can run just these three suites with the command:
npm run test-jasmine -- axes lib_date localize

[TomDemulierChevret]

Well I had a problem on my first try, but I will try again tomorrow.

[TomDemulierChevret]

Sorry, didn't have time to check this issue on Friday.

Regarding axes_test, the issue was with the initialisation of ax in setConvert method.
The fullLayout parameter passed to it has always an _extraFormat member from now on (retrieved from the locale) but since it's manually created in the test, this member was missing.

Regarding lib_date_test, the issue was that method formatDate was called directly without the new extraFormat parameter (which is normally passed via ax like in the axes_test).
I looked throught the code and couldn't find an occurence of formatDate called in another place than axes.js (which provides the extraFormat paramater as stated before).
Therefore I added manually the extraFormat to formatDate calls in lib_date_test when needed.

But since my knowledge of plotly.js is pretty limited, I may have missed some usage of formatDate, so you should definitly check there isn't a broken call somewhere.

I will try to add new test in localize_test to cover the new fields.

[TomDemulierChevret]

Just added a new test which check that both default locale (en) & provided locale format correctly the auto-formatted x-axis date tick.

Do you feel it is enough or should I add more test case ?

If everything is okay, can this PR be added to the next release ?

[alexcjohnson]

@TomDemulierChevret looks great - thanks for the fixes, and the new test covers it all nicely.

I'm re-running the tests - we've had a lot of spurious failures lately just when running the tests on CI, we're working on making those more robust - assuming it passes (eventually) I think this is ready to go! 💃

I'm going to wait a few days before merging, in case we need a patch release since we just put out several major new features. But yes, this will be in 1.34.0 🎉

[TomDemulierChevret]

Perfect !
I'm not really in a hurry to have this merged, it was just to know that it's planned.
I'm gonna use plotly in an upcomming project in which locale support is mandatory, given it seems to be the best JS chart/plot library it would've been a shame to not use it because of locale. ;)

[TomDemulierChevret]

Features, Bug fixes, and others:

This pull request add the possibility to localize the auto-formatted x-axis date ticks.
It countains a working example for the french locale.

If some (or all) of the new formats are missing in the locale set by the user, format from the en locale will be used (as it does for the base d3 format and for the translation keys).

[etpinard]

Thanks for the PR. Looks like @alexcjohnson was planning on doing this, but left it as a TODO.

A few tests are failing. I suspect formatDate is getting called outside of formatWorld w/o the extraFormat argument causing it to break.

[alexcjohnson]

modDateFormat calls out to components/calendars to get the full spectrum of d3-to-world-cal conversions - you've got the ones needed so far, but others will likely show up, and no need to reinvent the wheel with formatWorld.

At one point there was a performance argument for the structure we have here because we could use precompiled formatters... but we lost that benefit with the original date localization PR #2207, so now I think there would be a much more concise way to do this, something like:

calendar=isWorldCalendar(calendar)&&calendar;if(!fmt){if(tr==='y')fmt=extraFormat.year;elseif(tr==='m')fmt=extraFormat.month;elseif(tr==='d'){fmt=extraFormat.dayMonth+'\n'+extraFormat.year;}else{returnmodDateFormat(extraFormat.dayMonthYear,x,formatter,calendar)+'\n'+formatTime(x,tr);}}returnmodDateFormat(fmt,x,formatter,calendar);

[TomDemulierChevret]

Yeah, I suspected that this conversion table existed somewhere but I didn't know exactly where to look (and code search with date/format/etc returned way too many entries).

I like the new proposal, pretty clear to read.

[TomDemulierChevret]

Just test and it does work indeed on my example (only the last else is incorrect, need to switch date and time).

[alexcjohnson]

%e is padded with a space for 1-digit days - we should switch back to %-d, which has no padding (in fr.js as well). That's responsible for the image test failures.

[alexcjohnson]

@TomDemulierChevret are you able to get the tests to run locally? It looks like axes_test and lib_date_test are both failing, @etpinard probably had the right idea there. We should also add a couple of explicit tests in localize_test to cover the newly added fields.

You can run just these three suites with the command:
npm run test-jasmine -- axes lib_date localize

[TomDemulierChevret]

Well I had a problem on my first try, but I will try again tomorrow.

[TomDemulierChevret]

Sorry, didn't have time to check this issue on Friday.

Regarding axes_test, the issue was with the initialisation of ax in setConvert method.
The fullLayout parameter passed to it has always an _extraFormat member from now on (retrieved from the locale) but since it's manually created in the test, this member was missing.

Regarding lib_date_test, the issue was that method formatDate was called directly without the new extraFormat parameter (which is normally passed via ax like in the axes_test).
I looked throught the code and couldn't find an occurence of formatDate called in another place than axes.js (which provides the extraFormat paramater as stated before).
Therefore I added manually the extraFormat to formatDate calls in lib_date_test when needed.

But since my knowledge of plotly.js is pretty limited, I may have missed some usage of formatDate, so you should definitly check there isn't a broken call somewhere.

I will try to add new test in localize_test to cover the new fields.

[TomDemulierChevret]

Just added a new test which check that both default locale (en) & provided locale format correctly the auto-formatted x-axis date tick.

Do you feel it is enough or should I add more test case ?

If everything is okay, can this PR be added to the next release ?

[alexcjohnson]

@TomDemulierChevret looks great - thanks for the fixes, and the new test covers it all nicely.

I'm re-running the tests - we've had a lot of spurious failures lately just when running the tests on CI, we're working on making those more robust - assuming it passes (eventually) I think this is ready to go! 💃

I'm going to wait a few days before merging, in case we need a patch release since we just put out several major new features. But yes, this will be in 1.34.0 🎉

[TomDemulierChevret]

Perfect !
I'm not really in a hurry to have this merged, it was just to know that it's planned.
I'm gonna use plotly in an upcomming project in which locale support is mandatory, given it seems to be the best JS chart/plot library it would've been a shame to not use it because of locale. ;)

[TomDemulierChevret]

Features, Bug fixes, and others:

This pull request add the possibility to localize the auto-formatted x-axis date ticks.
It countains a working example for the french locale.

If some (or all) of the new formats are missing in the locale set by the user, format from the en locale will be used (as it does for the base d3 format and for the translation keys).

[etpinard]

Thanks for the PR. Looks like @alexcjohnson was planning on doing this, but left it as a TODO.

A few tests are failing. I suspect formatDate is getting called outside of formatWorld w/o the extraFormat argument causing it to break.

[alexcjohnson]

modDateFormat calls out to components/calendars to get the full spectrum of d3-to-world-cal conversions - you've got the ones needed so far, but others will likely show up, and no need to reinvent the wheel with formatWorld.

At one point there was a performance argument for the structure we have here because we could use precompiled formatters... but we lost that benefit with the original date localization PR #2207, so now I think there would be a much more concise way to do this, something like:

calendar=isWorldCalendar(calendar)&&calendar;if(!fmt){if(tr==='y')fmt=extraFormat.year;elseif(tr==='m')fmt=extraFormat.month;elseif(tr==='d'){fmt=extraFormat.dayMonth+'\n'+extraFormat.year;}else{returnmodDateFormat(extraFormat.dayMonthYear,x,formatter,calendar)+'\n'+formatTime(x,tr);}}returnmodDateFormat(fmt,x,formatter,calendar);

[TomDemulierChevret]

Yeah, I suspected that this conversion table existed somewhere but I didn't know exactly where to look (and code search with date/format/etc returned way too many entries).

I like the new proposal, pretty clear to read.

[TomDemulierChevret]

Just test and it does work indeed on my example (only the last else is incorrect, need to switch date and time).

[alexcjohnson]

%e is padded with a space for 1-digit days - we should switch back to %-d, which has no padding (in fr.js as well). That's responsible for the image test failures.

[alexcjohnson]

@TomDemulierChevret are you able to get the tests to run locally? It looks like axes_test and lib_date_test are both failing, @etpinard probably had the right idea there. We should also add a couple of explicit tests in localize_test to cover the newly added fields.

You can run just these three suites with the command:
npm run test-jasmine -- axes lib_date localize

[TomDemulierChevret]

Well I had a problem on my first try, but I will try again tomorrow.

[TomDemulierChevret]

Sorry, didn't have time to check this issue on Friday.

Regarding axes_test, the issue was with the initialisation of ax in setConvert method.
The fullLayout parameter passed to it has always an _extraFormat member from now on (retrieved from the locale) but since it's manually created in the test, this member was missing.

Regarding lib_date_test, the issue was that method formatDate was called directly without the new extraFormat parameter (which is normally passed via ax like in the axes_test).
I looked throught the code and couldn't find an occurence of formatDate called in another place than axes.js (which provides the extraFormat paramater as stated before).
Therefore I added manually the extraFormat to formatDate calls in lib_date_test when needed.

But since my knowledge of plotly.js is pretty limited, I may have missed some usage of formatDate, so you should definitly check there isn't a broken call somewhere.

I will try to add new test in localize_test to cover the new fields.

[TomDemulierChevret]

Just added a new test which check that both default locale (en) & provided locale format correctly the auto-formatted x-axis date tick.

Do you feel it is enough or should I add more test case ?

If everything is okay, can this PR be added to the next release ?

[alexcjohnson]

@TomDemulierChevret looks great - thanks for the fixes, and the new test covers it all nicely.

I'm re-running the tests - we've had a lot of spurious failures lately just when running the tests on CI, we're working on making those more robust - assuming it passes (eventually) I think this is ready to go! 💃

I'm going to wait a few days before merging, in case we need a patch release since we just put out several major new features. But yes, this will be in 1.34.0 🎉

[TomDemulierChevret]

Perfect !
I'm not really in a hurry to have this merged, it was just to know that it's planned.
I'm gonna use plotly in an upcomming project in which locale support is mandatory, given it seems to be the best JS chart/plot library it would've been a shame to not use it because of locale. ;)