Handle weekday strings in rangebreaks #4661

archmoj · 2020-03-18T17:17:05Z

Follow up of #4614 and #4655

convert string bounds e.g. Monday (or 'mon`) to day of week indices between [0, 6].
auto default to day of week when using day strings e.g. Monday or 'mon`.
update description
add jasmine tests

@plotly/plotly_js

src/plots/cartesian/layout_attributes.js

alexcjohnson · 2020-03-18T17:39:10Z

src/plots/cartesian/axis_defaults.js

@@ -161,6 +163,27 @@ function rangebreaksDefaults(itemIn, itemOut, containerOut) {
                itemOut.bounds = itemOut.bounds.slice(0, 2);


I don't think you have to do this slice(0, 2) - coerce for info_array already does this.
But if somehow I'm mistaken about that, you'd need to also shorten bnds so you don't lengthen itemOut.bounds again in the loops below.

I don't see any place where itemOut.bounds.length is changed.

itemOut.bounds[i] = bnds[i] = q;

Good call. I revised the new logic in 550a375.

Again, you don't need to worry about the length of bnds at all - coerce handles that so bnds (and itemOut.bounds) is either undefined (since it has no default value) or it's an array of length 2 - or less, I just tested this, if you pass in a shorter array coerce isn't going to pad it since the inner values don't have defaults either, but that's OK, it'll just look like undefined for missing values and get filtered out in the type checks.

Can you please take all these length checks out? We put a lot of effort into making coerce do the right thing so the individual supplyDefaults functions could be simple and readable.

Good call. Addressed in 84c2a6f.

alexcjohnson · 2020-03-18T17:46:50Z

src/plots/cartesian/axis_defaults.js

+
+function indexOfDay(v) {
+    var str = String(v).substr(0, 3).toLowerCase();
+    return weekSTR.indexOf(str);


This could be sped up a bit with an object lookup rather than indexOf

var dayStrToNum = { sun: 0, mon: 1, ... } return dayStrToNum[str];

and possibly bail out early if v isn't already a string, rather than converting to a string only to discard it later.

Good calls! Done in db2c3a3.
Curious to know if there is a good article on this?
At the moment, there are quite a number of places that indexOf !== -1 is applied, so please feel free to open an issue for that.
Thanks.

Not sure about an article... you find a lot of people saying "hash lookup is O(1) but indexOf is O(n)" but that's as far as it usually goes.

Here's a jsperf showing the general trends: https://jsperf.com/array-indexof-vs-object-key-lookup2/12 - basically, object lookups hardly scale with the number of keys but indexOf scales linearly. So indexOf can be a nice way to simplify if (a===b || a===c || a===d) -> if ([b,c,d].indexOf(a) !== -1) - but more than a few items and it's better to turn the problem into an object lookup.

Also of course object lookups only support strings... looking forward to when we stop supporting IE and can use Set!

@alexcjohnson thank you!!

src/plots/cartesian/axis_defaults.js

…urn and non-strict condition checks

alexcjohnson · 2020-03-18T18:21:33Z

src/plots/cartesian/axis_defaults.js

@@ -189,3 +210,21 @@ function rangebreaksDefaults(itemIn, itemOut, containerOut) {
        }
    }
 }
+
+// these numbers are one more than what bounds would be mapped to


clever way to avoid having to distinguish 0 from undefined :)

src/plots/cartesian/axis_defaults.js

Co-Authored-By: alexcjohnson <johnson.alex.c@gmail.com>

…g bounds length

nicolaskruchten · 2020-03-18T19:54:49Z

I <3 this!! cc @chriddyp

alexcjohnson · 2020-03-19T01:38:22Z

src/plots/cartesian/axis_defaults.js

+                        case DAY_OF_WEEK :
+                            if(isNumeric(q)) q = Math.floor(q);
+
+                            q = Math.floor(+q);


redundant - and dangerous - q='' would be turned into 0 for example. We should only cast to number when the value has already passed isNumeric (but might be a numeric string).

Also I don't think we should floor, we should just discard values that aren't integers (ie enabled=false). For one thing, that way if we ever add support for fractional days it won't break any existing behavior.

Can't this numeric validation be pushed into the if(pattern === DAY_OF_WEEK) loop above, and only executed if we didn't already convert from a day name to integer?

Good call. Fixed in 9a0416a.

alexcjohnson

Alright, I think we've got it! 💃

archmoj added 2 commits March 18, 2020 12:51

Handle day of week str - add auto dflt for pattern

a03d64d

add tests to handle weekday strings

d52a803

archmoj added feature something new status: reviewable labels Mar 18, 2020

archmoj added this to the v1.53.0 milestone Mar 18, 2020

archmoj requested review from alexcjohnson and nicolaskruchten March 18, 2020 17:17

alexcjohnson reviewed Mar 18, 2020

View reviewed changes

src/plots/cartesian/layout_attributes.js Show resolved Hide resolved

alexcjohnson reviewed Mar 18, 2020

View reviewed changes

mention matching logic in description

05ab23c

alexcjohnson reviewed Mar 18, 2020

View reviewed changes

src/plots/cartesian/axis_defaults.js Show resolved Hide resolved

attempt improve performance (a bit) by using object lookup, early ret…

db2c3a3

…urn and non-strict condition checks

alexcjohnson reviewed Mar 18, 2020

View reviewed changes

src/plots/cartesian/axis_defaults.js Outdated Show resolved Hide resolved

archmoj and others added 2 commits March 18, 2020 14:24

no need to cast string to string

c48f551

Co-Authored-By: alexcjohnson <johnson.alex.c@gmail.com>

improve matching - only need to test first two bounds - avoid touchin…

550a375

…g bounds length

archmoj added 2 commits March 18, 2020 16:03

validate numberic bounds when using patterns

e481a13

simplify default logic

84c2a6f

alexcjohnson reviewed Mar 19, 2020

View reviewed changes

correct axes default logic

9a0416a

alexcjohnson approved these changes Mar 19, 2020

View reviewed changes

archmoj merged commit d91e017 into master Mar 19, 2020

archmoj deleted the rangebreaks-handle-weekday-str branch March 19, 2020 12:45

snyk-bot mentioned this pull request Apr 23, 2020

[Snyk] Upgrade plotly.js from 1.52.1 to 1.53.0 mc-ireiser/vue-plotly#1

Open

snyk-bot mentioned this pull request May 1, 2020

[Snyk] Upgrade plotly.js-basic-dist from 1.52.3 to 1.53.0 libamtrack/web#583

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle weekday strings in rangebreaks #4661

Handle weekday strings in rangebreaks #4661

archmoj commented Mar 18, 2020

alexcjohnson Mar 18, 2020

archmoj Mar 18, 2020

alexcjohnson Mar 18, 2020

archmoj Mar 18, 2020

alexcjohnson Mar 18, 2020

archmoj Mar 18, 2020

alexcjohnson Mar 18, 2020

alexcjohnson Mar 18, 2020

archmoj Mar 18, 2020

alexcjohnson Mar 19, 2020

archmoj Mar 19, 2020

alexcjohnson Mar 18, 2020

nicolaskruchten commented Mar 18, 2020

alexcjohnson Mar 19, 2020

archmoj Mar 19, 2020

alexcjohnson left a comment

		@@ -161,6 +163,27 @@ function rangebreaksDefaults(itemIn, itemOut, containerOut) {
		itemOut.bounds = itemOut.bounds.slice(0, 2);

Handle weekday strings in rangebreaks #4661

Handle weekday strings in rangebreaks #4661

Conversation

archmoj commented Mar 18, 2020

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

nicolaskruchten commented Mar 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcjohnson left a comment

Choose a reason for hiding this comment