"calendars" component #1230

etpinard · 2016-12-06T21:56:58Z

to be merged in #1220

This PR makes @alexcjohnson's world-calendars a requirable component in plotly.js/lib/. This new "calendars" component will be included by default only in the main plotly.js bundle. The changes proposed here effectively 🔪 my concerns in #1220 (review)

Off this branch:

That the ~200K world-calendars module is only bundled in dist/plotly.js, the other plotly.js bundle show little 🍔 increase.

- use Registry in lib.dates.js to invoke world-calendar logic on-demand.

- ... only when calendars is registered. - replace 'calendar' valType with the appropriate 'enumarated' val object -

- replace layoutNodes list with _module.schema object which allow multiple distinct attribute (e.g. with different description) for each nodes

- ++ allow trace and transform attributes to be filled by component modules.

etpinard · 2016-12-06T21:58:45Z

src/constants/numerical.js

+     * For fast conversion btwn world calendars and epoch ms, the Julian Day Number
+     * of the unix epoch. From calendars.instance().newDate(1970, 1, 1).toJD()
+     */
+    EPOCHJD: 2440587.5


I'm not super happy about this, but I had to find a way to make EPOCHJD requirable in both lib/dates.js and components/calendars/ in a non-circular way

If the whole contents of the if(calendar) blocks went into the component, you wouldn't need EPOCHJD in dates at all, would you?

Sure. This will lead to some return object destructuring though. I'll see how bad it gets.

I'm not sure I want to go through with this.

Moving all the logic from the if(calendar) blocks to the calendars component would involved passing oddly named variables to new routines and return sets of values and even mutating them in findExactDates.

I understand that if would be nice to put all the world-calendar logic in one place, but it would make the code less readable, unless we start duplicating some dates.js logic in the calendars component.

The current implementation is both DRY and readable - which gets my vote.

Sure, lets leave it as is.

etpinard · 2016-12-06T21:59:16Z

src/lib/dates.js

+            Registry.getComponentMethod('calendars', 'CANONICAL_TICK')[calendar];
+    }
+    else {
+        return sunday ? '2000-01-02' : '2000-01-01';


maybe these gregorian defaults should be placed elsewhere ...

etpinard · 2016-12-06T21:59:59Z

src/components/calendars/index.js

@@ -16,6 +16,25 @@ var constants = require('../../constants/numerical');
 var EPOCHJD = constants.EPOCHJD;
 var ONEDAY = constants.ONEDAY;

+var attributes = {
+    valType: 'enumerated',


no need for a calendar valType anymore!

etpinard · 2016-12-06T22:00:30Z

src/traces/box/defaults.js

-    coerce('xcalendar', dfltCalendar);
-    coerce('ycalendar', dfltCalendar);
+    var handleCalendarDefaults = Registry.getComponentMethod('calendars', 'handleTraceDefaults');
+    handleCalendarDefaults(traceIn, traceOut, ['x', 'y'], layout);


a little ugly, but Registry gets it done

etpinard · 2016-12-06T22:01:43Z

src/components/calendars/index.js

 module.exports = {
    moduleType: 'component',
    name: 'calendars',

+    schema: {


some fancy (slightly hacky) declaration to make sure that calendar attributes will show up on https://community.plot.ly/

how will we know to keep this up to date?

how will we know to keep this up to date?

good question. There's got to be a way to test this.

alexcjohnson · 2016-12-07T15:18:30Z

src/plot_api/plot_api.js

@@ -312,7 +312,7 @@ Plotly.plot = function(gd, data, layout, config) {

        // show annotations and shapes
        Registry.getComponentMethod('shapes', 'draw')(gd);
-        Registry.getComponentMethod('annoations', 'draw')(gd);


how did this not get caught before?

Not sure. Annotations.draw gets called twice in Plotly.plot. Maybe one too many?

That second Annotations.draw call was added in #243 here.

I wonder if we can run (most of) our tests in a mode where all components are registered, and Registry.getComponentMethod throws an error if it doesn't find what it's looking for, rather than returning noop?

I wonder if we can run (most of) our tests in a mode where all components are registered

That's currently the case as most tests have var Plotly = require('@lib/index.js') which registers all traces and components.

Registry.getComponentMethod throws an error if it doesn't find what it's looking for, rather than returning noop?

Yeah, we could mutate the Registry export in the test suite. Let me try that.

cool - not in this PR though, unless it's super easy :)

alexcjohnson · 2016-12-07T15:23:24Z

src/traces/histogram/defaults.js

@@ -26,6 +27,9 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout
    var x = coerce('x'),
        y = coerce('y');

+    var handleCalendarDefaults = Registry.getComponentMethod('calendars', 'handleTraceDefaults');
+    handleCalendarDefaults(traceIn, traceOut, ['x', 'y'], layout);
+


previously we coerced calendars after the return on no data - was it intentional to move this up?

Oops. My bad.

done in cb2c54b

alexcjohnson · 2016-12-07T18:50:04Z

@etpinard this looks ready to me! Remaining comments are nonblocking from my perspective 💃

etpinard added 7 commits December 6, 2016 15:38

move world-calendar logic in lib/dates.js to new 'calendars' component

3143099

- use Registry in lib.dates.js to invoke world-calendar logic on-demand.

coerce calendar attributes in calenders component

5984106

- ... only when calendars is registered. - replace 'calendar' valType with the appropriate 'enumarated' val object -

add requirable 'calendars' module + include it in main bundle

03ab34f

fix typo in getComponentMethod call

db3d18b

skip over calendar attribute in findArrayAttributes

05b2f96

generalise plot schema handling of component attributes

61ecd42

- replace layoutNodes list with _module.schema object which allow multiple distinct attribute (e.g. with different description) for each nodes

add schema attributes in calendars module

68af287

- ++ allow trace and transform attributes to be filled by component modules.

etpinard added status: reviewable feature something new labels Dec 6, 2016

etpinard added this to the v1.21.0 milestone Dec 6, 2016

etpinard commented Dec 6, 2016

View reviewed changes

alexcjohnson reviewed Dec 7, 2016

View reviewed changes

move calendar defaults after early return

cb2c54b

etpinard merged commit bc457a9 into world-cals Dec 7, 2016

etpinard deleted the world-cals-component branch December 7, 2016 19:28

etpinard mentioned this pull request Dec 7, 2016

use only the calendars we need from 'world-calendars' #1237

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

"calendars" component #1230

"calendars" component #1230

etpinard commented Dec 6, 2016

etpinard Dec 6, 2016

alexcjohnson Dec 6, 2016

etpinard Dec 7, 2016

etpinard Dec 7, 2016

alexcjohnson Dec 7, 2016

etpinard Dec 6, 2016

etpinard Dec 6, 2016

etpinard Dec 6, 2016

etpinard Dec 6, 2016

alexcjohnson Dec 7, 2016

etpinard Dec 7, 2016

alexcjohnson Dec 7, 2016

etpinard Dec 7, 2016

etpinard Dec 7, 2016 •

edited

Loading

alexcjohnson Dec 7, 2016

etpinard Dec 7, 2016

alexcjohnson Dec 7, 2016

alexcjohnson Dec 7, 2016

etpinard Dec 7, 2016

etpinard Dec 7, 2016

alexcjohnson commented Dec 7, 2016

"calendars" component #1230

"calendars" component #1230

Conversation

etpinard commented Dec 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard Dec 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcjohnson commented Dec 7, 2016

etpinard Dec 7, 2016 •

edited

Loading