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

Update dates helper #828

Merged
merged 4 commits into from
Aug 10, 2023
Merged

Update dates helper #828

merged 4 commits into from
Aug 10, 2023

Conversation

gazayas
Copy link
Contributor

@gazayas gazayas commented Aug 8, 2023

Continuation of #696.

Submitting as draft since I still have a little bit of work to do in the joint PR, bullet-train-co/bullet_train-core#182

@gazayas
Copy link
Contributor Author

gazayas commented Aug 8, 2023

@jagthedrummer I updated the tests to use page.execute_script from #811 for the new date-related fields.

Besides that, I got the Super Scaffolding tests to pass by using a fix to transformer which I temporarily added to bullet-train-co/bullet_train-core#182. I'll revert that here now that the tests are passing.

The transformer fix can be found here: bullet-train-co/bullet_train-core#386

@gazayas
Copy link
Contributor Author

gazayas commented Aug 8, 2023

@kaspth I also went ahead and used assert_text like you mentioned here 👍

@gazayas gazayas marked this pull request as ready for review August 8, 2023 10:03
Copy link
Contributor

@kaspth kaspth left a comment

Choose a reason for hiding this comment

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

Code LGTM

@jagthedrummer jagthedrummer merged commit 8aa2b59 into main Aug 10, 2023
@jagthedrummer jagthedrummer deleted the standardize-time-locales branch August 10, 2023 14:34
@jagthedrummer
Copy link
Contributor

@gazayas After merging this we have one failure in CI when running against the released version of the BT gems. And I didn't realize until after I'd merged it that this branch hadn't run against both the released version and core/main.

The failure is:

expected to find text "Thu, 10 Aug 2023 14:52:00 +0000"

And what's actually there is:

Today at 2:52

Is that expected? And is it what we want? Today at 2:52 seems much more user friendly to me than Thu, 10 Aug 2023 14:52:00 +0000.

@gazayas
Copy link
Contributor Author

gazayas commented Aug 10, 2023

@jagthedrummer About the strings like "Today at 2:52", I expressed here that we would trade this off in favor of the Rails default:

We unfortunately lose the "Today at ..." and "Yesterday at ..." strings we once had, but I think this is a good trade off because we can then write our partials like this:

<%= render 'shared/attributes/date', attribute: :test_date %> <!-- Uses default format -->
OR
<%= render 'shared/attributes/date', attribute: :test_date, format: :short %>
OR
<%= render 'shared/attributes/date', attribute: :test_date, format: '%m/%d/%Y' %>

Because of that, I was under the assumption that we would be ok with the change. What do you think is the best approach? Would be glad to look over the tests or whatever's necessary.

@jagthedrummer
Copy link
Contributor

@gazayas Ah, I see. I think I overlooked that particular detail in all of the convo.

🤔 Just thinking out loud, but is there a way that we could provide a default translation string that preserves the previous behavior so that we're not changing things out from under people?

For instance is there a way to somehow do this?

en:
  time:
    formats:
      bt_default: "Today at #{...}"

And then have the helper default to using the bt_default translation?

Or if not that, could we bring back the previous way that we calculated that string and continue to use it as the default if the dev doesn't provide a format?

Or maybe a config option that can be set (which we maybe turn on by default)?

It just seems very unfriendly to change the default format that people get without notice, and with no obvious/easy way to get the old defaults back.

jagthedrummer added a commit that referenced this pull request Aug 11, 2023
@gazayas
Copy link
Contributor Author

gazayas commented Aug 14, 2023

@jagthedrummer, I saw that you opened #844, LGTM

jagthedrummer added a commit that referenced this pull request Aug 14, 2023
* Revert "Update dates helper (#828)"

This reverts commit 8aa2b59.

* use assert_text to get better error messages
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