Skip to content

fix(ui): keep the gap between date and time segments in every locale - #16794

Open
thomassarazin wants to merge 2 commits into
medusajs:developfrom
thomassarazin:fix/ui-date-segment-locale-separator
Open

fix(ui): keep the gap between date and time segments in every locale#16794
thomassarazin wants to merge 2 commits into
medusajs:developfrom
thomassarazin:fix/ui-date-segment-locale-separator

Conversation

@thomassarazin

@thomassarazin thomassarazin commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Summary

What — In @medusajs/ui, a DatePicker with a time granularity renders the date glued to the time in French and in many other locales: 09/09/202609:05 instead of 09/09/2026 09:05. This PR keeps the gap whatever the locale.

WhyDateSegment special-cases the literal segment between the date and the time, but only when its text is the en-US ", ": that literal is replaced by an empty <span className="mx-1" />, which is what produces the visible gap. In fr-FR (and de-DE, es-ES, it-IT, nl-NL…), Intl.DateTimeFormat emits a lone " " as that literal. It falls through to the generic branch and is rendered as a text node made only of whitespace, which the browser collapses at the line edge. The gap disappears and the two groups of digits touch. The admin dashboard shows this on every date-with-time field (price list start and end dates, campaign dates, scheduled publishing) as soon as the browser language is not English.

How — The special case now applies to the en-US ", " literal as before and to any literal whose trimmed text is empty, so the space used by the other locales takes the same spacing span. One condition changed, no new prop, no layout change for en-US.

Testing — Added date-segment.spec.tsx: renders a DatePicker with granularity="minute" under I18nProvider in en-US and fr-FR, and asserts that a spacing span exists and that no separator survives as text (no leaf node equal to ", ", none made only of whitespace). Node's ICU honours the locale here (formatToParts gives ", " for en-US and " " for fr-FR). The fr-FR case fails on the original code and passes after the fix; the en-US case guards against dropping the comma branch. Also verified manually on a 2.15.5 admin with the browser in French: price list dates read 09/09/2026 09:05 after the fix. Bug reproduced on develop as well.


Examples

<I18nProvider locale="fr-FR">
  <DatePicker granularity="minute" value={new Date(2026, 8, 9, 9, 5)} />
</I18nProvider>
// before: 09/09/202609:05
// after:  09/09/2026 09:05

Checklist

  • I have added a changeset for this PR
  • The changes are covered by relevant tests
  • I have verified the code works as intended locally
  • I have linked the related issue(s) if applicable — no existing issue found for this bug

Additional Context

Found while scheduling sales on a French store: the start and end dates of a price list were unreadable in the admin.

🤖 Generated with Claude Code

https://claude.ai/code/session_0135G2otNMNJ8kQ9Gk7wzsqo

The literal segment between the date and the time was only special-cased
when its text is the en-US ", ". In fr-FR and many other locales that
literal is a lone " ", rendered as a whitespace-only text node that the
browser collapses at the line edge: the date and the time were glued
together (09/09/202609:05). Any literal whose trimmed text is empty now
takes the same spacing span.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0135G2otNMNJ8kQ9Gk7wzsqo
@thomassarazin
thomassarazin requested review from a team as code owners September 11, 2026 12:06
@changeset-bot

changeset-bot Bot commented Sep 11, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 066f158

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 83 packages
Name Type
@medusajs/ui Patch
@medusajs/draft-order Patch
@medusajs/loyalty-plugin Patch
@medusajs/dashboard Patch
@medusajs/medusa Patch
@medusajs/admin-bundler Patch
@medusajs/test-utils Patch
@medusajs/medusa-oas-cli Patch
integration-tests-http Patch
@medusajs/analytics Patch
@medusajs/api-key Patch
@medusajs/auth Patch
@medusajs/caching Patch
@medusajs/cart Patch
@medusajs/currency Patch
@medusajs/customer Patch
@medusajs/file Patch
@medusajs/fulfillment Patch
@medusajs/index Patch
@medusajs/inventory Patch
@medusajs/link-modules Patch
@medusajs/locking Patch
@medusajs/notification Patch
@medusajs/order Patch
@medusajs/payment Patch
@medusajs/pricing Patch
@medusajs/product Patch
@medusajs/promotion Patch
@medusajs/rbac Patch
@medusajs/region Patch
@medusajs/sales-channel Patch
@medusajs/search Patch
@medusajs/settings Patch
@medusajs/stock-location Patch
@medusajs/store Patch
@medusajs/tax Patch
@medusajs/translation Patch
@medusajs/user Patch
@medusajs/workflow-engine-inmemory Patch
@medusajs/workflow-engine-redis Patch
@medusajs/search-postgres Patch
@medusajs/oas-github-ci Patch
@medusajs/cache-inmemory Patch
@medusajs/cache-redis Patch
@medusajs/event-bus-local Patch
@medusajs/event-bus-redis Patch
@medusajs/analytics-local Patch
@medusajs/analytics-posthog Patch
@medusajs/auth-emailpass Patch
@medusajs/auth-github Patch
@medusajs/auth-google Patch
@medusajs/auth-oidc Patch
@medusajs/caching-redis Patch
@medusajs/file-local Patch
@medusajs/file-s3 Patch
@medusajs/fulfillment-manual Patch
@medusajs/locking-postgres Patch
@medusajs/locking-redis Patch
@medusajs/notification-local Patch
@medusajs/notification-sendgrid Patch
@medusajs/payment-stripe Patch
@medusajs/core-flows Patch
@medusajs/framework Patch
@medusajs/instantsearch-adapter Patch
@medusajs/js-sdk Patch
@medusajs/modules-sdk Patch
@medusajs/orchestration Patch
@medusajs/query Patch
@medusajs/types Patch
@medusajs/utils Patch
@medusajs/workflows-sdk Patch
create-medusa-app Patch
@medusajs/http-types-generator Patch
@medusajs/cli Patch
@medusajs/deps Patch
@medusajs/eslint-plugin Patch
@medusajs/telemetry Patch
@medusajs/admin-sdk Patch
@medusajs/admin-shared Patch
@medusajs/admin-vite-plugin Patch
@medusajs/icons Patch
@medusajs/toolbox Patch
@medusajs/ui-preset Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@medusa-os-bot

medusa-os-bot Bot commented Sep 11, 2026

Copy link
Copy Markdown

Thanks for the contribution! A few items need to be addressed before this can move forward:

The contributor fixes missing spacing between the date and time segments of DatePicker in non-English locales. The approach is to widen the literal-segment check from the exact string ", " to any literal whose trimmed text is empty. A changeset (correct patch bump, correct fix(ui): format) and a parametrised test covering en-US and fr-FR are included. There is a correctness problem with the condition. ", ".trim() evaluates to "," — the comma is not whitespace, so it is not stripped, and the result is non-empty. The new condition segment.text.trim() === "" therefore does NOT match the en-US ", " separator. In real browsers where the date–time literal is ", " (as the original code was explicitly written to handle, and as the test's own block comment states), the new code falls through to the default branch and renders ", " as visible text inside a <div>, losing the mx-1 spacing span — a regression for English-locale users. The test most likely passes because the jsdom environment does not accurately replicate locale-aware ICU formatting, making the en-US assertion a false positive rather than genuine coverage of real-browser behaviour.

  • packages/design-system/ui/src/components/date-segment/date-segment.tsx, isEmptyLiteral condition: logic error — ", ".trim() is "," (non-empty), so the en-US date–time separator ", " does not satisfy segment.text.trim() === "". In real browsers the mx-1 span is therefore never rendered for en-US, causing a visual regression (the comma becomes visible text and the margin gap disappears). Fix: combine both cases — segment.text === ", " || segment.text.trim() === "" — to preserve the original en-US handling while extending it to space-only separators used by other locales.
  • packages/design-system/ui/src/components/date-segment/date-segment.spec.tsx, block comment inside the test: the comment states that en-US uses ", " as the literal, yet the test asserts span.mx-1 is present for en-US. Under the new condition these two claims are mutually exclusive — if en-US really uses ", ", the span is not rendered. The test is likely a false positive because jsdom does not honour the locale passed to I18nProvider for ICU formatting. The test should be updated to reflect what the environment actually produces, or a note should explain that the jsdom output differs from real-browser output for en-US.

Triggered by: new PR opened

", ".trim() is ",", so the widened condition dropped the original en-US
case. The spacing span now applies to the ", " literal as before and to
whitespace-only literals. The test asserts that no separator survives as
text, which fails for en-US with the previous condition and for fr-FR
with the original code.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0135G2otNMNJ8kQ9Gk7wzsqo
@thomassarazin

Copy link
Copy Markdown
Contributor Author

Good catch, thank you. ", ".trim() is ",", so the widened condition dropped the en-US case. Fixed in 066f158:

  • the spacing span now applies to the ", " literal as before and to whitespace-only literals: segment.text === ", " || segment.text.trim() === "";
  • the test no longer only checks that a span exists. It asserts that no separator survives as text: no leaf node with ", ", none made only of whitespace. With that assertion, the previous condition fails on en-US and the original code fails on fr-FR (verified locally by swapping the component back in);
  • about jsdom: Node's ICU does honour the locale here. formatToParts gives ["/", "/", ", ", ":", " "] for en-US and ["/", "/", " ", ":"] for fr-FR, which is also what the segments render. The earlier en-US pass was a false positive for a different reason: the " " before the day period matched the trim-only condition, so a span existed even though the comma was left as text. The updated assertion catches exactly that.

One side effect worth noting: the " " before AM/PM in en-US was also rendered as a collapsing text node before this change, so 12-hour locales get their gap before the day period too.

@medusa-os-bot

medusa-os-bot Bot commented Sep 11, 2026

Copy link
Copy Markdown

Thanks for the contribution! Initial automated review looks good.

The contributor fixes a locale-specific visual bug in the DatePicker component where the date and time segments were rendered without a gap in non-English locales (e.g., fr-FR, de-DE). The root cause was that the original code only matched the en-US literal separator ", ", while other locales emit a bare " " (space-only) which the browser collapses, causing the two groups of digits to touch. A previous review flagged a logic error in the first attempt — ", ".trim() evaluates to "," (non-empty), so the en-US case was inadvertently dropped. The author addressed this correctly: the condition now reads segment.text === ", " || segment.text.trim() === "", which preserves the original en-US branch and adds handling for space-only separators. The fix also benefits 12-hour locales where a " " before AM/PM was also rendered as a collapsing text node. The test covers both en-US and fr-FR, asserting that a spacing span exists and that no leaf div contains ", " or non-empty whitespace-only text. The logic is sound and correctly distinguishes the two cases. Checklist: PR template complete, changeset included with correct patch bump and fix(ui): format, tests added, no issue/PR references in code comments, no security or performance concerns.

Triggered by: new commit pushed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant