Skip to content

Conversation

@charliepark
Copy link
Contributor

We're working to tighten up the data tables (vertically), so are moving the date cell from two lines to one line.

Currently, it looks like this:
Screenshot 2024-04-16 at 12 07 04 PM

With this PR, it now looks like this:
Screenshot 2024-04-16 at 12 01 11 PM

On smaller screens, the date wraps:
Screenshot 2024-04-16 at 12 48 21 PM

@vercel
Copy link

vercel bot commented Apr 16, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
console ✅ Ready (Inspect) Visit Preview Apr 17, 2024 6:26am

@charliepark charliepark changed the title Single line date cell Convert date cell to single line Apr 16, 2024
<span>{format(date, 'PP')}</span>
<span className="text-quaternary">{format(date, 'p')}</span>
</time>
)
Copy link
Collaborator

@david-crespo david-crespo Apr 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm realizing date-fns is very literal and doesn't seem to do any locale-based formatting. I think we should use toLocaleString() and get decent results, i.e., the same as we're currently getting in English but better for other languages. Something like this:

const dateStr = date.toLocaleString(undefined, { month: 'short', day: 'numeric', year: 'numeric' })
const timeStr = date.toLocaleString(undefined, { hour: 'numeric', minute: 'numeric' }))
image

In the math function we use navigator.language where I've put undefined here, but it seems like undefined is really the right way. The only problem is that the way I've tested this is to mock navigator.language, so if we use undefined instead that approach won't work in the tests. Maybe we should be using playwright instead to test the locale rendering. They make it really easy:

import { test, expect } from '@playwright/test';

test.describe('french language block', () => {

  test.use({ locale: 'fr-FR' });

  test('example', async ({ page }) => {
    // ...
  });
});

https://playwright.dev/docs/test-use-options#configuration-scopes

To summarize, I think you should:

  1. Replace these date-fns format calls with toLocaleString(undefined, ...)
  2. Check a displayed date in exactly one table in de-DE or whatever using the Playwright local setting feature
  3. Don't worry about the number formatting functions, I'll make an issue to switch those to use undefined instead of navigator.language and change the way we test it

<div className="flex flex-col">
<span className="text-tertiary">{tooltipText}</span>
<span>{format(datetime, 'MMM d, yyyy p')}</span>
<span>{toLocaleDateString(datetime)}</span>
Copy link
Collaborator

@david-crespo david-crespo Apr 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should include the time too. You might need another helper , or maybe toLocaleString() by itself would do it? (nah).

main

Screenshot 2024-04-16 at 7 08 51 PM

This branch

Screenshot 2024-04-16 at 7 08 46 PM

Copy link
Contributor Author

@charliepark charliepark Apr 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(after updating) … The native formatting adds a comma after the year (for en-US), but I think it's fine
Screenshot 2024-04-16 at 8 40 20 PM

<span>{toLocaleDateString(date, locale)}</span>
<span className="text-quaternary">{toLocaleTimeString(date, locale)}</span>
</time>
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stylish

@charliepark
Copy link
Contributor Author

I have a few Playwright tests to update as well; coming soonish

@charliepark
Copy link
Contributor Author

Changing my system language to French worked correctly:
Capture d’écran 2024-04-16 à 20 10 38

And back to English:
Screenshot 2024-04-16 at 8 34 17 PM

Copy link
Collaborator

@david-crespo david-crespo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Tests should be simple, I think just looking at one page where there is a date should be fine.

app/util/date.ts Outdated
const timeOptions = {
hour: 'numeric',
minute: 'numeric',
} satisfies Intl.DateTimeFormatOptions
Copy link
Collaborator

@david-crespo david-crespo Apr 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh, check this out: I was just reading the MDN page and noticed the style shortcuts section: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/DateTimeFormat/DateTimeFormat#datestyle

I think we can reduce these to timeStyle: 'short' and dateStyle: 'medium'! That is clean.

const dateOptions = { dateStyle: 'medium' } satisfies Intl.DateTimeFormatOptions
const timeOptions = { timeStyle: 'short' } satisfies Intl.DateTimeFormatOptions
image

Oddly it does change both the German and Japanese results. Seems fine to me?

image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, interesting. Funny that it changes the German syntax. I don't have strong feelings on it, but agree the shorter configuration syntax is nice. Will get that in.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eh, I maybe prefer the output of the earlier version, but am fine either way, and recognize this will likely be an edge case for some time. We can always change it back if we decide that's better.
Screenshot 2024-04-16 at 9 32 25 PM

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though it looks worse (maybe) I like the idea of taking the details out of our hands — in theory they should know better than we do what’s idiomatic in each locale.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense; I agree with that.

@charliepark charliepark merged commit eeb32a7 into main Apr 17, 2024
@charliepark charliepark deleted the single-line-date-cell branch April 17, 2024 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants