Skip to content

BinSize adjustments - #813

Merged
VeraZab merged 10 commits into
masterfrom
binsize
Jan 3, 2019
Merged

BinSize adjustments#813
VeraZab merged 10 commits into
masterfrom
binsize

Conversation

@VeraZab

@VeraZab VeraZab commented Dec 17, 2018

Copy link
Copy Markdown
Contributor

So that it becomes this widget when axis type is date:
screen shot 2018-12-17 at 4 24 08 pm

@nicolaskruchten

Copy link
Copy Markdown
Contributor

I like the look! I'll let @dmt0 review the code

@VeraZab

VeraZab commented Dec 17, 2018

Copy link
Copy Markdown
Contributor Author

I'm not done : )

@VeraZab
VeraZab force-pushed the binsize branch 4 times, most recently from 9bfe067 to 1b5ea29 Compare December 17, 2018 21:23
@VeraZab

VeraZab commented Dec 17, 2018

Copy link
Copy Markdown
Contributor Author

@dmt0 ready for review, would you play around with this one a little and tell me if you find anything weird (@nicolaskruchten if you have a bit of time you too )

Basically, I've chosen to convert everything to the millisecond format in bin.size to simplify conversions, but the approximations I'm making do produce some minor oddities like, going from 12 months to days gives 360 days. Also I've put upper and lower boundaries for the days and months formats. So for days, I put 366, but maybe I shouldn't, (because those could be in other calendars for ex).

The percy diff is because I changed max bins and bin size widget's places, because I thought it looked a bit better visually, when the format was date.
screen shot 2018-12-17 at 4 32 55 pm

vs

screen shot 2018-12-17 at 4 34 45 pm

@nicolaskruchten

Copy link
Copy Markdown
Contributor

Why do you convert the M syntax to millis? My strong instinct is to expose it, as it's a native plotly.js feature.

@VeraZab

VeraZab commented Dec 17, 2018

Copy link
Copy Markdown
Contributor Author

Why do you convert the M syntax to millis?

Added back the native 'M' support

@dmt0

dmt0 commented Dec 18, 2018

Copy link
Copy Markdown
Contributor

Code looks fine, but it seems rather odd to me to have this specific subset of timeframes, I think users would find years useful for sure. Hours, minutes and seconds too.

Also if we are working with months, a month really can't be assumed to be 30 days. I think it makes sense to go through the trouble of doing things moment.js way. Convert things to moment and use those functions to add months.

@nicolaskruchten

Copy link
Copy Markdown
Contributor

@dmt0 plotly.js natively supports e.g. M3 to mean "3 months" no matter which months they are, so we need to expose that. Days is useful, and millisecond precision is supported, so we may as well include it. Years are just M12 or M24 etc. I could see a case for minutes.

@VeraZab

VeraZab commented Dec 18, 2018

Copy link
Copy Markdown
Contributor Author

Years are just M12 or M24 etc.

ok given above, I've removed the max bounds from this component.

I can add minutes, it will look like so:
screen shot 2018-12-18 at 9 16 08 am

@nicolaskruchten nicolaskruchten mentioned this pull request Dec 18, 2018
6 tasks
@nicolaskruchten

nicolaskruchten commented Dec 18, 2018

Copy link
Copy Markdown
Contributor
  • So can we make this a dropdown and have full words instead, and then add days, minutes, seconds, milliseconds?

  • Also, we'll need a display rule for first-render... if I give "12345" as the value, should that show as millis or seconds or...? probably "whatever units show a log value closest to 1" will be a good rule.

@VeraZab

VeraZab commented Dec 18, 2018

Copy link
Copy Markdown
Contributor Author

screen shot 2018-12-18 at 10 42 47 am

screen shot 2018-12-18 at 10 42 42 am

@dmt0

dmt0 commented Dec 18, 2018

Copy link
Copy Markdown
Contributor

I know that a year is just 12M, but I think it's good to include it in the dropdown

@nicolaskruchten

Copy link
Copy Markdown
Contributor

💃

@VeraZab
VeraZab merged commit 0e20d19 into master Jan 3, 2019
@VeraZab
VeraZab deleted the binsize branch January 3, 2019 16:37
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.

3 participants