Adding quarter to timeUnit for #947 #1305
Conversation
|
Looks like you are on the right track but there is still a lot missing. I suggest that you think of a few examples, write tests for those and see that you support them. For instance, the time unit can be |
| @@ -114,7 +117,10 @@ export function formatMixins(model: Model, channel: Channel, format: string) { | |||
| def.format = model.config().numberFormat; | |||
| break; | |||
| case TEMPORAL: | |||
| def.format = timeFormat(model, channel) || model.config().timeFormat; | |||
| if ((!fieldDef.timeUnit) || (fieldDef.timeUnit && | |||
| fieldDef.timeUnit.toString().indexOf('quarter') === -1)) { | |||
There was a problem hiding this comment.
Please write a util function for this? Something like hasTimeUnit(string, timeUnit).
There was a problem hiding this comment.
the second argument should be a timeUnit so you will need to convert it to a string. Let me know if you can't find out how to convert from a ts enum value to a string.
kanitw
changed the title
| @@ -103,7 +103,9 @@ export function formatMixins(model: Model, channel: Channel, format: string) { | |||
| let def: any = {}; | |||
|
|
|||
| if (fieldDef.type === TEMPORAL) { | |||
| def.formatType = 'time'; | |||
| if ((!fieldDef.timeUnit) || !containsTimeUnit(fieldDef.timeUnit, TimeUnit.QUARTER)) { | |||
There was a problem hiding this comment.
Can you comment why if there is no timeUnit or if the timeUnit is quarter we don't assign formatType here?
There was a problem hiding this comment.
I guess for quarter, we use format as a part of template?
|
LGTM. @bobocandys can you merge master into this so I can do a final review and merge? |
|
Test still failing |
…into ylin/addTimeUnit
Make sure text works for axis, label, legend and mark.
Use templates everywhere instead of format and formatType or template.
|
Only need to adapt to latest datalib changes. Then we're ready to go. Problem is that we also need to wait for a vega release that includes latest datalib changes. Note that the latest dl change is not yet in a release. |
|
@arvind Do you plan to release a Vega patch sometimes soon? (Two PR are blocked by the next Vega release.) |
|
There have been some features that have landed into Vega master, so I'm hesitant to cut a patch release. My goal is to release a minor version once Emily's themeing work has landed which should hopefully be within the week or two. If a patch release is urgent, I can create one by rolling back on master. |
|
@bobocandys Let's wait for the vega release. Until then the test should fail in this pr. |
|
Why is the test passing now? |
|
I'm not sure. I thought the tests should be failing because we use an unreleased feature (quarter filters in expressions) but we can detect the error only if we parse a spec with vega (expecting |
|
This should not be blocked any more. |
| const format = model.config().mark.format; | ||
| extend(p, formatMixins(model, TEXT, format)); | ||
| } else if (TEMPORAL === model.fieldDef(TEXT).type) { | ||
| p.text = { | ||
| // need to replace datum.data -> datum.FIELD_NAME |
There was a problem hiding this comment.
@domoritz this regexp looks complicated, not sure if we really need it.
| if (out.length > 0) { | ||
| // clean up quarter formatting remainders | ||
| const template = '{{' + field + ' | time:\'' + out.join(' ') + '\'}}'; | ||
| return template.replace(new RegExp('{{' + field + ' \\| time:\'\'}}', 'g'), ''); |
There was a problem hiding this comment.
@domoritz another regexp that I don't understsand ...
Make axis compatible with quarter as timeUnit.