Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

find occurence of time, improve time formatting js #1799

Merged
merged 4 commits into from
Apr 15, 2024
Merged

Conversation

amazingphilippe
Copy link
Contributor

@amazingphilippe amazingphilippe commented Apr 10, 2024

Summary | Résumé

Fixes cds-snc/notification-planning#571

Time strings are all <span> elements. They could be <time> elements.

Docs for <time>

We already transform raw datetime strings on the client side with javascript.

My approach is

  • Keep the client-side date formatting
  • In templates, we should use <time> wherever this client side formatting runs.
  • To avoid repetition in templates, we can set <time>'s datetime attribute with our client side script. It almost looks like a custom component.

Test instructions | Instructions pour tester la modification

  1. Find the 3 different types of time strings
  2. On a revoked API key item
  3. Check the created and expiry date
  4. Check the "revoked {date}" on the right
  5. On a job report
  6. Check the "available until {date}"

Copy link

Copy link
Member

@andrewleith andrewleith left a comment

Choose a reason for hiding this comment

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

It looks like you may have missed the datetimes with the .relative-time-past class:

There is also a utility function that seems to be used in the dashboard and on the login landing page that creates a span still:

Other than those, it looks like the datetime display is still behaving the same way it did before.

Copy link
Member

@andrewleith andrewleith left a comment

Choose a reason for hiding this comment

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

LGTM - works as expected in the PR review environment.

@amazingphilippe amazingphilippe merged commit 61ff00e into main Apr 15, 2024
9 checks passed
@amazingphilippe amazingphilippe deleted the a11y/time branch April 15, 2024 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Times and Dates
2 participants