fix: add support for formatType: "utc" #9265
Conversation
There was a problem hiding this comment.
I don't have cycles to look into this in detail but it seems like in the previous pull request there was something intentional about distinguishing utc and time: https://github.com/vega/vega-lite/pull/7815/files#diff-df52deef186d568b5237ce82ff44bba2a0bed4e69430fae0e55b3b2876068414R140. This needs more careful testing as we want to make sure that we apply the right formatting for the right scale type. Please add more tests and I'd like another set of eyes on this.
| it('should return existing format type', () => { | ||
| expect(guideFormatType('number', {field: ' foo', type: 'quantitative'}, 'ordinal')).toBe('number'); | ||
| expect(guideFormatType('time', {field: ' foo', type: 'quantitative'}, 'ordinal')).toBe('time'); | ||
| expect(guideFormatType('utc', {field: ' foo', type: 'quantitative'}, 'ordinal')).toBe('utc'); |
There was a problem hiding this comment.
Does this test the new code? I thought it's for nominal.
| export function isFieldOrDatumDefForTimeFormat(fieldOrDatumDef: FieldDef<string> | DatumDef): boolean { | ||
| const {formatType} = getFormatMixins(fieldOrDatumDef); | ||
| return formatType === 'time' || (!formatType && isTimeFieldDef(fieldOrDatumDef)); | ||
| return formatType === 'time' || formatType === 'utc' || (!formatType && isTimeFieldDef(fieldOrDatumDef)); |
There was a problem hiding this comment.
We use formatType === 'time' || formatType === 'utc' quite often. Can we pull it out into a reusable method?
|
|
||
| export function isCustomFormatType(formatType: string) { | ||
| return formatType && formatType !== 'number' && formatType !== 'time'; | ||
| return formatType && formatType !== 'number' && formatType !== 'time' && formatType !== 'utc'; |
| if (formatType && (isSignalRef(formatType) || formatType === 'number' || formatType === 'time')) { | ||
| if ( | ||
| formatType && | ||
| (isSignalRef(formatType) || formatType === 'number' || formatType === 'time' || formatType === 'utc') |
There was a problem hiding this comment.
and here (possible other places as well).
2 participants
Fixes #7795 for nominal fields