feat(playground): Use format API from client-core#10693
Conversation
github-actions
bot
added
client:core
|
Claude finished @ovr's task in 3m 25s —— View job Code Review: feat(playground): Use format API from client-core
Overall: Looks good. This is a well-structured consolidation that moves formatting logic out of the playground and into What I Like
Issues & Suggestions
Security & Performance
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if (text === null) return <Tag>NULL</Tag>; | ||
| if (typeof text === 'object') return <Tag>OBJECT</Tag>; | ||
| if (text === undefined) return ''; | ||
|
|
||
| return formatValue(text, { | ||
| type: c.type, | ||
| format: c.format, | ||
| currency: c.currency, | ||
| granularity: c.granularity, | ||
| }); |
There was a problem hiding this comment.
Nit (consistency): The non-granularity branch here now uses formatValue nicely, but the granularity branch above (lines 446–457, unchanged in this PR) still uses formatDateByGranularity directly with an isValidISOTimestamp guard and try/catch.
Since formatValue with type: 'time' internally delegates to formatDateByGranularity and already handles invalid dates gracefully (returns "Invalid date"), you could consider unifying both branches into a single render function to reduce the asymmetry. The granularity branch also doesn't handle null/undefined the way this branch does — if text is null, isValidISOTimestamp(null) returns false and the raw null passes through.
Not blocking, but worth considering for a follow-up.
| import formatDate from 'date-fns/format'; | ||
|
|
||
| const FORMAT_MAP = { | ||
| second: 'yyyy-LL-dd HH:mm:ss', | ||
| minute: 'yyyy-LL-dd HH:mm', | ||
| hour: 'yyyy-LL-dd HH:00', | ||
| day: 'yyyy-LL-dd', | ||
| week: "yyyy-LL-dd 'W'w", | ||
| month: 'yyyy LLL', | ||
| quarter: 'yyyy QQQ', | ||
| year: 'yyyy', | ||
| }; | ||
|
|
||
| export function formatDateByGranularity(timestamp: Date, granularity?: TimeDimensionGranularity) { | ||
| return formatDate( | ||
| timestamp, | ||
| FORMAT_MAP[(granularity as Exclude<TimeDimensionGranularity, string>) ?? 'second'] ?? | ||
| FORMAT_MAP['second'] | ||
| ); | ||
| } | ||
|
|
||
| export function formatDateByPattern(timestamp: Date, format?: string) { | ||
| return formatDate(timestamp, format ?? FORMAT_MAP['second']); | ||
| return formatDate(timestamp, format ?? 'yyyy-LL-dd HH:mm:ss'); | ||
| } |
There was a problem hiding this comment.
Nit: Now that formatDateByGranularity has been removed from this file, the filename format-date-by-granularity.tsx no longer accurately describes its content — it only exports formatDateByPattern. Consider renaming to format-date-by-pattern.tsx (and updating the re-export in utils/index.ts) to avoid confusion.
| export const formatD3NumericLocale: Record<string, Omit<FormatLocaleDefinition, 'currency'>> = { | ||
| 'en-US': { decimal: '.', thousands: ',', grouping: [3] }, | ||
| 'en-GB': { decimal: '.', thousands: ',', grouping: [3] }, | ||
| 'zh-CN': { decimal: '.', thousands: ',', grouping: [3] }, | ||
| 'es-ES': { decimal: ',', thousands: '.', grouping: [3] }, | ||
| 'es-MX': { decimal: '.', thousands: ',', grouping: [3] }, | ||
| 'de-DE': { decimal: ',', thousands: '.', grouping: [3] }, | ||
| 'ja-JP': { decimal: '.', thousands: ',', grouping: [3] }, | ||
| 'fr-FR': { decimal: ',', thousands: '\u00a0', grouping: [3], percent: '\u202f%' }, | ||
| 'pt-BR': { decimal: ',', thousands: '.', grouping: [3] }, | ||
| 'ko-KR': { decimal: '.', thousands: ',', grouping: [3] }, | ||
| 'it-IT': { decimal: ',', thousands: '.', grouping: [3] }, | ||
| 'nl-NL': { decimal: ',', thousands: '.', grouping: [3] }, | ||
| 'ru-RU': { decimal: ',', thousands: '\u00a0', grouping: [3] }, | ||
| }; |
There was a problem hiding this comment.
Good change — inlining only the needed properties (decimal, thousands, grouping) instead of importing full JSON locale files from d3-format reduces bundle size and makes the type more accurate with Omit<FormatLocaleDefinition, 'currency'> since currency is always resolved dynamically via getCurrencyOverride.
| console.warn('Failed to generate d3 local via Intl, failing back to en-US', e); | ||
|
|
||
| definition = formatD3NumericLocale['en-US']; | ||
| definition = { | ||
| ...formatD3NumericLocale['en-US'], | ||
| currency: getCurrencyOverride(locale, currencyCode) | ||
| }; | ||
| } |
There was a problem hiding this comment.
Good fix — the fallback path previously used the bare formatD3NumericLocale['en-US'] without currency resolution. Now it properly applies getCurrencyOverride, ensuring currency symbols are correct even in the error fallback.
| const DATETIME_FORMAT_BY_GRANULARITY: Record<string, string> = { | ||
| second: '%Y-%m-%d %H:%M:%S', | ||
| minute: '%Y-%m-%d %H:%M', | ||
| hour: '%Y-%m-%d %H:00', | ||
| day: '%Y-%m-%d', | ||
| week: '%Y-%m-%d W%V', | ||
| month: '%Y %b', | ||
| quarter: '%Y Q%q', | ||
| year: '%Y', | ||
| }; |
There was a problem hiding this comment.
The format patterns were changed for several granularities compared to the previous version:
minute:%Y-%m-%d %H:%M:%S→%Y-%m-%d %H:%M(drops seconds — good)hour:%Y-%m-%d %H:%M:%S→%Y-%m-%d %H:00(shows truncated hour — good)week:%Y-%m-%d→%Y-%m-%d W%V(adds ISO week number — more informative)month:%Y-%m→%Y %b(abbreviated month name — more readable)quarter:%Y-Q%q→%Y Q%q(space instead of dash)
These are user-visible formatting changes and improvements. They match what the playground previously had in its own FORMAT_MAP (date-fns patterns). Just flagging that any consumers of the old formatValue with time types will see different output.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #10693 +/- ##
=======================================
Coverage 58.03% 58.03%
=======================================
Files 215 215
Lines 16683 16683
Branches 3357 3357
=======================================
Hits 9682 9682
Misses 6511 6511
Partials 490 490
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
No description provided.