fix(#9430): improve format for axis with chronological timeUnit (time…

…Unit with year), by using Vega's default
vega · Sep 11, 2024 · b253f14 · b253f14
1 parent 802d661
commit b253f14
Show file tree

Hide file tree

Showing 3 changed files with 21 additions and 12 deletions.
diff --git a/src/compile/format.ts b/src/compile/format.ts
@@ -179,7 +179,7 @@ export function guideFormat(
   format: string | Dict<unknown>,
   formatType: string | SignalRef,
   config: Config,
-  omitTimeFormatConfig: boolean // axis doesn't use config.timeFormat
+  isAxis: boolean
 ) {
   if (isString(formatType) && isCustomFormatType(formatType)) {
     return undefined; // handled in encode block
@@ -216,7 +216,7 @@ export function guideFormat(
       return undefined; // hanlded in encode block
     }
 
-    return timeFormat({specifiedFormat: format as string, timeUnit, config, omitTimeFormatConfig});
+    return timeFormat({specifiedFormat: format as string, timeUnit, config, isAxis});
   }
 
   return numberFormat({type, specifiedFormat: format, config});
@@ -269,24 +269,25 @@ export function timeFormat({
   specifiedFormat,
   timeUnit,
   config,
-  omitTimeFormatConfig
+  isAxis
 }: {
   specifiedFormat?: string;
   timeUnit?: TimeUnit;
   config: Config;
-  omitTimeFormatConfig?: boolean;
+  isAxis?: boolean;
 }) {
   if (specifiedFormat) {
     return specifiedFormat;
   }
 
   if (timeUnit) {
     return {
-      signal: timeUnitSpecifierExpression(timeUnit)
+      signal: timeUnitSpecifierExpression(timeUnit, {isAxis})
     };
   }
 
-  return omitTimeFormatConfig ? undefined : config.timeFormat;
+  // axis doesn't use config.timeFormat
+  return isAxis ? undefined : config.timeFormat;
 }
 
 function formatExpr(field: string, format: string) {

diff --git a/src/timeunit.ts b/src/timeunit.ts
@@ -338,12 +338,20 @@ export function fieldExpr(fullTimeUnit: TimeUnit, field: string, {end}: {end: bo
   return dateTimeExprToExpr(dateExpr);
 }
 
-export function timeUnitSpecifierExpression(timeUnit: TimeUnit) {
+export function timeUnitSpecifierExpression(timeUnit: TimeUnit, {isAxis}: {isAxis: boolean}): string {
   if (!timeUnit) {
     return undefined;
   }
 
   const timeUnitParts = getTimeUnitParts(timeUnit);
+  if (isAxis) {
+    if (timeUnitParts.includes('year')) {
+      // If the timeUnit includes year (meaning it's a chronological timeUnit (aka datetime truncation),
+      // then the default axis format is actually pretty smart.
+      return undefined;
+    }
+  }
+
   return `timeUnitSpecifier(${stringify(timeUnitParts)}, ${stringify(VEGALITE_TIMEFORMAT)})`;
 }
 
@@ -355,7 +363,7 @@ export function formatExpression(timeUnit: TimeUnit, field: string, isUTCScale:
     return undefined;
   }
 
-  const expr = timeUnitSpecifierExpression(timeUnit);
+  const expr = timeUnitSpecifierExpression(timeUnit, {isAxis: false});
 
   // We only use utcFormat for utc scale
   // For utc time units, the data is already converted as a part of timeUnit transform.

diff --git a/test/compile/format.test.ts b/test/compile/format.test.ts
@@ -105,13 +105,13 @@ describe('Format', () => {
       });
     });
 
-    it('omits the timeFormat when omitTimeFormatConfig and no specifiedFormat', () => {
-      const formatted = timeFormat({config: {timeFormat: '%y'}, omitTimeFormatConfig: true});
+    it('omits the timeFormat when isAxis and no specifiedFormat', () => {
+      const formatted = timeFormat({config: {timeFormat: '%y'}, isAxis: true});
       expect(formatted).toBeUndefined();
     });
 
-    it('returns the timeFormat when !omitTimeFormatConfig and no specifiedFormat', () => {
-      const formatted = timeFormat({config: {timeFormat: '%y'}, omitTimeFormatConfig: false});
+    it('returns the timeFormat when !isAxis and no specifiedFormat', () => {
+      const formatted = timeFormat({config: {timeFormat: '%y'}, isAxis: false});
       expect(formatted).toBe('%y');
     });
   });