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

Remove special-casing of dates for slider formatting and fix datetimes #1122

Merged
merged 3 commits into from
Jan 26, 2024

Conversation

mikmart
Copy link
Contributor

@mikmart mikmart commented Jan 25, 2024

Follow-up to #1119.

This PR removes the now-unnecessary special-casing from formatDate() as the mechanism from #1119 handles it directly.

This also fixes a regression introduced in #1119 for datetime columns, which was caused by me overlooking to handle datetime values when applying a renderer to format the slider range labels.

There's some duplicated logic in the JS side formatDate() helper and restore(). I thought about refactoring that, but it would be a slightly more involved operation without a big direct benefit, so I decided not to mess with it at this point.

I tested manually with this app:

DT::datatable(
  data.frame(
    date = Sys.Date() + 1:10,
    time = Sys.time() + 1:10 * 60 * 60 * 24
  ),
  filter = "top"
) |> DT::formatDate(1:2, "toLocaleString")

Copy link
Member

@yihui yihui left a comment

Choose a reason for hiding this comment

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

Excellent! Thanks!

@yihui yihui merged commit 8dca7c4 into rstudio:main Jan 26, 2024
10 checks passed
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.

2 participants