Revise flags of rangebreak pattern #4653
Conversation
| ' breaks from Saturday to Monday (i.e. skips the weekends).', | ||
| '- { pattern: \'%H\', bounds: [17, 8] }', | ||
| ' breaks from 5pm to 8am (i.e. skips non-work hours).' | ||
| '- { pattern: \'time of day\', bounds: [16, 8] }', |
There was a problem hiding this comment.
bounds should be [18, 8] ?
There was a problem hiding this comment.
It seems reasonable (to me) to set operation' dfltback to "()" for thetime of day` case.
| '- { pattern: \'%H\', bounds: [17, 8] }', | ||
| ' breaks from 5pm to 8am (i.e. skips non-work hours).' | ||
| '- { pattern: \'time of day\', bounds: [16, 8] }', | ||
| ' breaks from 4pm to 8am (i.e. skips non-work hours).' |
There was a problem hiding this comment.
I reverted the hour example and added operation fix in 94f8ef8.
We will come back to finalize this when revising defaults in the following PR.
Let's leave the values as you have them here - I don't want to accept two values that do the same thing just because we can't agree which one is best 😄 |
In general I tend towards a Pythonic "there should be one way of doing things" and "that one way should bias towards being easy to read rather than easy to write whenever they are in tension" so I'm definitely against something like |
|
Per Slack, |
Done in d82db2e. |
| 'These are the same directive as in `tickformat`, see', | ||
| 'https://github.com/d3/d3-time-format#locale_format', | ||
| 'If *day of week* - Sunday-based weekday as a decimal number [0, 6].', | ||
| 'If *hour* - hour (24-hour clock) as a decimal number [0, 23].', |
There was a problem hiding this comment.
this accepts numbers from 0 to 23.999999 right?
There was a problem hiding this comment.
Not sure.
cc: @alexcjohnson
There was a problem hiding this comment.
let's check in on the time-strings here too plz. I don't mind if they're not supported but if they are we should leave it in and document it. :)
There was a problem hiding this comment.
Fractional hours worked in my testing, and time strings did not, but the gold standard is adding a test :)
There was a problem hiding this comment.
@archmoj your call whether to add these tests here or in another PR but they should be somewhere before the release.
There was a problem hiding this comment.
I am adding them in this PR.
| 'for more info.', | ||
| 'Examples:', | ||
| '- { pattern: \'%w\', bounds: [6, 0], operation: \'[]\' }', | ||
| '- { pattern: \'day of week\', bounds: [6, 0] }', |
There was a problem hiding this comment.
Does this PR implement the default operation?
There was a problem hiding this comment.
No. Those changes would be done in a separate PR.
|
@alexcjohnson I think this PR is provided the task it was supposed to do i.e. renaming those two flags. |
|
@alexcjohnson this PR is ready for the second round review. |
| 'These are the same directive as in `tickformat`, see', | ||
| 'https://github.com/d3/d3-time-format#locale_format', | ||
| 'If *' + DAY_OF_WEEK + '* - Sunday-based weekday as a decimal number [0, 6].', | ||
| 'If *' + HOUR + '* - hour (24-hour clock) as decimal numbers between 0 and 24.', |
There was a problem hiding this comment.
Oh we should note here that weekday needs to be an integer but hour can be fractional.
Follow up of #4614 .
This PR changes
'%w'to'day of week'and replaces
'%H'with'hour'.Fixes bug for decimal bounds: before vs after
cc: @plotly/plotly_js | @nicolaskruchten