Add new formatting options by replacing `d3.format` method with more recent `d3-format` module by archmoj · Pull Request #5125 · plotly/plotly.js

archmoj · 2020-09-04T03:53:49Z

An attempt to replace old d3 format functions with latest d3-number module as part of d3 bump cc: #424; while keep trimming the default option which was quite tricky.

TODO:

test more cases to ensure old options still work
test new options
add fall back for the edge cases
drop d3.format from plotly/d3 fork and use it here

Also to note:

dropped d3.time from plotly/d3 fork and switched to d3-time module in dc0d9cb cc: Bumping d3 #424

@plotly/plotly_js

PR to help maintain previous behavior: https://github.com/plotly/plotly.js/pull/5820/files
Added new functionalities illustrated in lib_number_format_test.js785f128
Also see extra options tested in #5842.

nicolaskruchten · 2020-09-04T13:45:28Z

Cool! Does this sidestep the challenges encountered in #5023 ?

archmoj · 2020-09-04T16:17:27Z

Cool! Does this sidestep the challenges encountered in #5023 ?

Positive. #5023 was not successful.

- centralize and wrap require d3 format in lib - adjust old and new formats

archmoj · 2021-07-12T19:45:13Z

 },
 "dependencies": {
- "@plotly/d3": "3.7.0",
+ "@plotly/d3": "git://github.com/plotly/d3.git#3fc195e30e85b008238783379d2aa067c737e636",


plotly/d3#4

Co-authored-by: Alex Johnson <johnson.alex.c@gmail.com>

alexcjohnson · 2021-07-17T04:41:56Z

 };

+lib.beginCap = function(str) {
+ return str.charAt(0).toUpperCase() + str.slice(1);


plotly.js/src/lib/index.js
Lines 723 to 725 in bdb69fc
lib.titleCase=function(s){
returns.charAt(0).toUpperCase()+s.substr(1);
};

Done in 1efc41e

alexcjohnson · 2021-07-17T04:48:22Z

+ Lib.adjustFormat(formatStr)
+ );
+ } catch(e) {
+ return Lib.noFormat;


Is this catching invalid format strings? Do we want to lib.warn when this happens, to help people fix it?

Done in 878034b

alexcjohnson

💃 after those couple of comments and merging/publishing plotly/d3#4

Nice job with lib.adjustFormat!

archmoj · 2020-09-04T03:53:49Z

An attempt to replace old d3 format functions with latest d3-number module as part of d3 bump cc: #424; while keep trimming the default option which was quite tricky.

TODO:

test more cases to ensure old options still work
test new options
add fall back for the edge cases
drop d3.format from plotly/d3 fork and use it here

Also to note:

dropped d3.time from plotly/d3 fork and switched to d3-time module in dc0d9cb cc: Bumping d3 #424

@plotly/plotly_js

PR to help maintain previous behavior: https://github.com/plotly/plotly.js/pull/5820/files
Added new functionalities illustrated in lib_number_format_test.js785f128
Also see extra options tested in #5842.

nicolaskruchten · 2020-09-04T13:45:28Z

Cool! Does this sidestep the challenges encountered in #5023 ?

archmoj · 2020-09-04T16:17:27Z

Cool! Does this sidestep the challenges encountered in #5023 ?

Positive. #5023 was not successful.

- centralize and wrap require d3 format in lib - adjust old and new formats

archmoj · 2021-07-12T19:45:13Z

 },
 "dependencies": {
- "@plotly/d3": "3.7.0",
+ "@plotly/d3": "git://github.com/plotly/d3.git#3fc195e30e85b008238783379d2aa067c737e636",


plotly/d3#4

Co-authored-by: Alex Johnson <johnson.alex.c@gmail.com>

alexcjohnson · 2021-07-17T04:41:56Z

 };

+lib.beginCap = function(str) {
+ return str.charAt(0).toUpperCase() + str.slice(1);


plotly.js/src/lib/index.js
Lines 723 to 725 in bdb69fc
lib.titleCase=function(s){
returns.charAt(0).toUpperCase()+s.substr(1);
};

Done in 1efc41e

alexcjohnson · 2021-07-17T04:48:22Z

+ Lib.adjustFormat(formatStr)
+ );
+ } catch(e) {
+ return Lib.noFormat;


Is this catching invalid format strings? Do we want to lib.warn when this happens, to help people fix it?

Done in 878034b

alexcjohnson

💃 after those couple of comments and merging/publishing plotly/d3#4

Nice job with lib.adjustFormat!

archmoj · 2020-09-04T03:53:49Z

An attempt to replace old d3 format functions with latest d3-number module as part of d3 bump cc: #424; while keep trimming the default option which was quite tricky.

TODO:

test more cases to ensure old options still work
test new options
add fall back for the edge cases
drop d3.format from plotly/d3 fork and use it here

Also to note:

dropped d3.time from plotly/d3 fork and switched to d3-time module in dc0d9cb cc: Bumping d3 #424

@plotly/plotly_js

PR to help maintain previous behavior: https://github.com/plotly/plotly.js/pull/5820/files
Added new functionalities illustrated in lib_number_format_test.js785f128
Also see extra options tested in #5842.

nicolaskruchten · 2020-09-04T13:45:28Z

Cool! Does this sidestep the challenges encountered in #5023 ?

archmoj · 2020-09-04T16:17:27Z

Cool! Does this sidestep the challenges encountered in #5023 ?

Positive. #5023 was not successful.

- centralize and wrap require d3 format in lib - adjust old and new formats

archmoj · 2021-07-12T19:45:13Z

 },
 "dependencies": {
- "@plotly/d3": "3.7.0",
+ "@plotly/d3": "git://github.com/plotly/d3.git#3fc195e30e85b008238783379d2aa067c737e636",


plotly/d3#4

Co-authored-by: Alex Johnson <johnson.alex.c@gmail.com>

alexcjohnson · 2021-07-17T04:41:56Z

 };

+lib.beginCap = function(str) {
+ return str.charAt(0).toUpperCase() + str.slice(1);


plotly.js/src/lib/index.js
Lines 723 to 725 in bdb69fc
lib.titleCase=function(s){
returns.charAt(0).toUpperCase()+s.substr(1);
};

Done in 1efc41e

alexcjohnson · 2021-07-17T04:48:22Z

+ Lib.adjustFormat(formatStr)
+ );
+ } catch(e) {
+ return Lib.noFormat;


Is this catching invalid format strings? Do we want to lib.warn when this happens, to help people fix it?

Done in 878034b

alexcjohnson

💃 after those couple of comments and merging/publishing plotly/d3#4

Nice job with lib.adjustFormat!

archmoj · 2020-09-04T03:53:49Z

An attempt to replace old d3 format functions with latest d3-number module as part of d3 bump cc: #424; while keep trimming the default option which was quite tricky.

TODO:

test more cases to ensure old options still work
test new options
add fall back for the edge cases
drop d3.format from plotly/d3 fork and use it here

Also to note:

dropped d3.time from plotly/d3 fork and switched to d3-time module in dc0d9cb cc: Bumping d3 #424

@plotly/plotly_js

PR to help maintain previous behavior: https://github.com/plotly/plotly.js/pull/5820/files
Added new functionalities illustrated in lib_number_format_test.js785f128
Also see extra options tested in #5842.

nicolaskruchten · 2020-09-04T13:45:28Z

Cool! Does this sidestep the challenges encountered in #5023 ?

archmoj · 2020-09-04T16:17:27Z

Cool! Does this sidestep the challenges encountered in #5023 ?

Positive. #5023 was not successful.

- centralize and wrap require d3 format in lib - adjust old and new formats

archmoj · 2021-07-12T19:45:13Z

 },
 "dependencies": {
- "@plotly/d3": "3.7.0",
+ "@plotly/d3": "git://github.com/plotly/d3.git#3fc195e30e85b008238783379d2aa067c737e636",


plotly/d3#4

Co-authored-by: Alex Johnson <johnson.alex.c@gmail.com>

alexcjohnson · 2021-07-17T04:41:56Z

 };

+lib.beginCap = function(str) {
+ return str.charAt(0).toUpperCase() + str.slice(1);


plotly.js/src/lib/index.js
Lines 723 to 725 in bdb69fc
lib.titleCase=function(s){
returns.charAt(0).toUpperCase()+s.substr(1);
};

Done in 1efc41e

alexcjohnson · 2021-07-17T04:48:22Z

+ Lib.adjustFormat(formatStr)
+ );
+ } catch(e) {
+ return Lib.noFormat;


Is this catching invalid format strings? Do we want to lib.warn when this happens, to help people fix it?

Done in 878034b

alexcjohnson

💃 after those couple of comments and merging/publishing plotly/d3#4

Nice job with lib.adjustFormat!

archmoj · 2020-09-04T03:53:49Z

An attempt to replace old d3 format functions with latest d3-number module as part of d3 bump cc: #424; while keep trimming the default option which was quite tricky.

TODO:

test more cases to ensure old options still work
test new options
add fall back for the edge cases
drop d3.format from plotly/d3 fork and use it here

Also to note:

dropped d3.time from plotly/d3 fork and switched to d3-time module in dc0d9cb cc: Bumping d3 #424

@plotly/plotly_js

PR to help maintain previous behavior: https://github.com/plotly/plotly.js/pull/5820/files
Added new functionalities illustrated in lib_number_format_test.js785f128
Also see extra options tested in #5842.

nicolaskruchten · 2020-09-04T13:45:28Z

Cool! Does this sidestep the challenges encountered in #5023 ?

archmoj · 2020-09-04T16:17:27Z

Cool! Does this sidestep the challenges encountered in #5023 ?

Positive. #5023 was not successful.

- centralize and wrap require d3 format in lib - adjust old and new formats

archmoj · 2021-07-12T19:45:13Z

 },
 "dependencies": {
- "@plotly/d3": "3.7.0",
+ "@plotly/d3": "git://github.com/plotly/d3.git#3fc195e30e85b008238783379d2aa067c737e636",


plotly/d3#4

Co-authored-by: Alex Johnson <johnson.alex.c@gmail.com>

alexcjohnson · 2021-07-17T04:41:56Z

 };

+lib.beginCap = function(str) {
+ return str.charAt(0).toUpperCase() + str.slice(1);


plotly.js/src/lib/index.js
Lines 723 to 725 in bdb69fc
lib.titleCase=function(s){
returns.charAt(0).toUpperCase()+s.substr(1);
};

Done in 1efc41e

alexcjohnson · 2021-07-17T04:48:22Z

+ Lib.adjustFormat(formatStr)
+ );
+ } catch(e) {
+ return Lib.noFormat;


Is this catching invalid format strings? Do we want to lib.warn when this happens, to help people fix it?

Done in 878034b

alexcjohnson

💃 after those couple of comments and merging/publishing plotly/d3#4

Nice job with lib.adjustFormat!

archmoj · 2020-09-04T03:53:49Z

An attempt to replace old d3 format functions with latest d3-number module as part of d3 bump cc: #424; while keep trimming the default option which was quite tricky.

TODO:

test more cases to ensure old options still work
test new options
add fall back for the edge cases
drop d3.format from plotly/d3 fork and use it here

Also to note:

dropped d3.time from plotly/d3 fork and switched to d3-time module in dc0d9cb cc: Bumping d3 #424

@plotly/plotly_js

PR to help maintain previous behavior: https://github.com/plotly/plotly.js/pull/5820/files
Added new functionalities illustrated in lib_number_format_test.js785f128
Also see extra options tested in #5842.

nicolaskruchten · 2020-09-04T13:45:28Z

Cool! Does this sidestep the challenges encountered in #5023 ?

archmoj · 2020-09-04T16:17:27Z

Cool! Does this sidestep the challenges encountered in #5023 ?

Positive. #5023 was not successful.

- centralize and wrap require d3 format in lib - adjust old and new formats

archmoj · 2021-07-12T19:45:13Z

 },
 "dependencies": {
- "@plotly/d3": "3.7.0",
+ "@plotly/d3": "git://github.com/plotly/d3.git#3fc195e30e85b008238783379d2aa067c737e636",


plotly/d3#4

Co-authored-by: Alex Johnson <johnson.alex.c@gmail.com>

alexcjohnson · 2021-07-17T04:41:56Z

 };

+lib.beginCap = function(str) {
+ return str.charAt(0).toUpperCase() + str.slice(1);


plotly.js/src/lib/index.js
Lines 723 to 725 in bdb69fc
lib.titleCase=function(s){
returns.charAt(0).toUpperCase()+s.substr(1);
};

Done in 1efc41e

alexcjohnson · 2021-07-17T04:48:22Z

+ Lib.adjustFormat(formatStr)
+ );
+ } catch(e) {
+ return Lib.noFormat;


Is this catching invalid format strings? Do we want to lib.warn when this happens, to help people fix it?

Done in 878034b

alexcjohnson

💃 after those couple of comments and merging/publishing plotly/d3#4

Nice job with lib.adjustFormat!

archmoj · 2020-09-04T03:53:49Z

An attempt to replace old d3 format functions with latest d3-number module as part of d3 bump cc: #424; while keep trimming the default option which was quite tricky.

TODO:

test more cases to ensure old options still work
test new options
add fall back for the edge cases
drop d3.format from plotly/d3 fork and use it here

Also to note:

dropped d3.time from plotly/d3 fork and switched to d3-time module in dc0d9cb cc: Bumping d3 #424

@plotly/plotly_js

PR to help maintain previous behavior: https://github.com/plotly/plotly.js/pull/5820/files
Added new functionalities illustrated in lib_number_format_test.js785f128
Also see extra options tested in #5842.

nicolaskruchten · 2020-09-04T13:45:28Z

Cool! Does this sidestep the challenges encountered in #5023 ?

archmoj · 2020-09-04T16:17:27Z

Cool! Does this sidestep the challenges encountered in #5023 ?

Positive. #5023 was not successful.

- centralize and wrap require d3 format in lib - adjust old and new formats

archmoj · 2021-07-12T19:45:13Z

 },
 "dependencies": {
- "@plotly/d3": "3.7.0",
+ "@plotly/d3": "git://github.com/plotly/d3.git#3fc195e30e85b008238783379d2aa067c737e636",


plotly/d3#4

Co-authored-by: Alex Johnson <johnson.alex.c@gmail.com>

alexcjohnson · 2021-07-17T04:41:56Z

 };

+lib.beginCap = function(str) {
+ return str.charAt(0).toUpperCase() + str.slice(1);


plotly.js/src/lib/index.js
Lines 723 to 725 in bdb69fc
lib.titleCase=function(s){
returns.charAt(0).toUpperCase()+s.substr(1);
};

Done in 1efc41e

alexcjohnson · 2021-07-17T04:48:22Z

+ Lib.adjustFormat(formatStr)
+ );
+ } catch(e) {
+ return Lib.noFormat;


Is this catching invalid format strings? Do we want to lib.warn when this happens, to help people fix it?

Done in 878034b

alexcjohnson

💃 after those couple of comments and merging/publishing plotly/d3#4

Nice job with lib.adjustFormat!

archmoj · 2020-09-04T03:53:49Z

An attempt to replace old d3 format functions with latest d3-number module as part of d3 bump cc: #424; while keep trimming the default option which was quite tricky.

TODO:

test more cases to ensure old options still work
test new options
add fall back for the edge cases
drop d3.format from plotly/d3 fork and use it here

Also to note:

dropped d3.time from plotly/d3 fork and switched to d3-time module in dc0d9cb cc: Bumping d3 #424

@plotly/plotly_js

PR to help maintain previous behavior: https://github.com/plotly/plotly.js/pull/5820/files
Added new functionalities illustrated in lib_number_format_test.js785f128
Also see extra options tested in #5842.

nicolaskruchten · 2020-09-04T13:45:28Z

Cool! Does this sidestep the challenges encountered in #5023 ?

archmoj · 2020-09-04T16:17:27Z

Cool! Does this sidestep the challenges encountered in #5023 ?

Positive. #5023 was not successful.

- centralize and wrap require d3 format in lib - adjust old and new formats

archmoj · 2021-07-12T19:45:13Z

 },
 "dependencies": {
- "@plotly/d3": "3.7.0",
+ "@plotly/d3": "git://github.com/plotly/d3.git#3fc195e30e85b008238783379d2aa067c737e636",


plotly/d3#4

Co-authored-by: Alex Johnson <johnson.alex.c@gmail.com>

alexcjohnson · 2021-07-17T04:41:56Z

 };

+lib.beginCap = function(str) {
+ return str.charAt(0).toUpperCase() + str.slice(1);


plotly.js/src/lib/index.js
Lines 723 to 725 in bdb69fc
lib.titleCase=function(s){
returns.charAt(0).toUpperCase()+s.substr(1);
};

Done in 1efc41e

alexcjohnson · 2021-07-17T04:48:22Z

+ Lib.adjustFormat(formatStr)
+ );
+ } catch(e) {
+ return Lib.noFormat;


Is this catching invalid format strings? Do we want to lib.warn when this happens, to help people fix it?

Done in 878034b

alexcjohnson

💃 after those couple of comments and merging/publishing plotly/d3#4

Nice job with lib.adjustFormat!

archmoj added bug something broken feature something new status: reviewable labels Sep 4, 2020

archmoj added status: in progress and removed status: reviewable labels Sep 24, 2020

archmoj force-pushed the new-d3-number-format branch 2 times, most recently from 9c20e84 to 427e5e3Compare November 9, 2020 16:17

archmoj force-pushed the new-d3-number-format branch from 427e5e3 to efaebd7Compare December 3, 2020 14:51

archmoj force-pushed the new-d3-number-format branch from efaebd7 to b59b1fbCompare January 5, 2021 17:22

archmoj force-pushed the new-d3-number-format branch from b59b1fb to fb0e62eCompare May 22, 2021 17:57

archmoj force-pushed the new-d3-number-format branch from fb0e62e to 9f51808Compare June 26, 2021 16:05

archmoj added this to the v2.3.0 milestone Jun 26, 2021

archmoj added the status: reviewable label Jun 26, 2021

archmoj force-pushed the new-d3-number-format branch 3 times, most recently from 9b69f1e to cd0d392Compare July 7, 2021 18:38

archmoj added 11 commits July 7, 2021 14:42

apply latest d3 number format instead of d3 v3 format
5092c19
- centralize and wrap require d3 format in lib - adjust old and new formats

update baselines
cd0d392

reflect schema changes
c7ecdc7

draft log for PR 5615
320b270

refactor adjustFormat
3ea8d90

refactor number format functions
a5fd000

Merge remote-tracking branch 'origin/master' into new-d3-number-format
6b93e61

catch and handle bad d3 number format instead of throwing error
25a30a8

point to d3-format v1.4.5 options not higher
05b221c

Merge remote-tracking branch 'origin/master' into new-d3-number-format
a5a94cd

link d3 v3 without d3.format
e88a134

archmoj mentioned this pull request Jul 12, 2021
Drop unused format functions from d3-v3 fork plotly/d3#4
Merged

archmoj commented Jul 12, 2021
View reviewed changes

archmoj changed the title ~~Upgrade d3 number formatting~~Add new formatting options by switching to d3-format moduleJul 12, 2021

archmoj changed the title ~~Add new formatting options by switching to d3-format module~~Add new formatting options by replacing d3.format method with more recent d3-format moduleJul 12, 2021

edit draft log
d5d282d

archmoj mentioned this pull request Jul 12, 2021
Add jasmine tests in respect to current d3.format options #5820
Merged

archmoj added 2 commits July 12, 2021 19:21

Merge remote-tracking branch 'origin/master' into new-d3-number-format
68fe2b7

cast to string in the case of bad format
17e73b6

archmoj removed the status: has TODOs label Jul 12, 2021

add jasmine tests
785f128

archmoj requested review from alexcjohnson and nicolaskruchten July 12, 2021 23:30

archmoj added 5 commits July 13, 2021 14:30

removed d3.time now switch to use d3-time
dc0d9cb

correct comment
a425925

centralized fn Lib.beginCap to work with new d3 methods
11a75d9

Merge remote-tracking branch 'origin/master' into new-d3-number-format
dc94854

Merge remote-tracking branch 'origin/master' into new-d3-number-format
bdb69fc

alexcjohnson reviewed Jul 17, 2021
View reviewed changes

Comment threadsrc/lib/index.js Outdated

Update src/lib/index.js
693ebd7
Co-authored-by: Alex Johnson <johnson.alex.c@gmail.com>

alexcjohnson reviewed Jul 17, 2021
View reviewed changes

alexcjohnson approved these changes Jul 17, 2021
View reviewed changes

archmoj added 5 commits July 17, 2021 00:59

use titleCase function
1efc41e

warn once encountered a bad format
878034b

Merge remote-tracking branch 'origin/master' into new-d3-number-format
80d37d2

update baseline using Raleway font instead of Dosis
518029a

bump plotly/d3 to v3.8.0
32898a1

archmoj merged commit 437f70e into masterJul 17, 2021

archmoj deleted the new-d3-number-format branch July 17, 2021 20:59

This was referenced Jul 20, 2021
No option for tickformat with '(' (parentheses) sign #5511
Closed
Add tests for padding and alignment without formatting numbers #5842
Merged