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

[SFT-604] Add Created and Updated users #942

Merged
merged 26 commits into from
Jan 11, 2024
Merged

[SFT-604] Add Created and Updated users #942

merged 26 commits into from
Jan 11, 2024

Conversation

aaronlinley
Copy link
Contributor

No description provided.

Copy link
Contributor

@kjmartens kjmartens left a comment

Choose a reason for hiding this comment

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

Looks really good. I tested upgrade as well as fresh install. Behavior seems correct. One improvement, however, would be to remove the link to other users if they don't have the Edit → Edit users Craft permission. Currently it just links to # which isn't the greatest. 🙂

Copy link
Contributor

@gustavs-gutmanis gustavs-gutmanis left a comment

Choose a reason for hiding this comment

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

Stellar!
Just two more small updates and we're all set here.

@aaronlinley
Copy link
Contributor Author

@gustavs-gutmanis @kjmartens All PR feedback has now been addressed

Copy link
Contributor

@kjmartens kjmartens left a comment

Choose a reason for hiding this comment

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

Looks good to me 👌

@kjmartens kjmartens self-requested a review January 5, 2024 17:59
Copy link
Contributor

@kjmartens kjmartens left a comment

Choose a reason for hiding this comment

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

One issue I noticed is that the time is not correctly localized to me. I am GTM-06 and it's currently 12:00 PM here, but the time saved shows 01/05/2024, 6:00 PM. When I change my timezone to another one, it changes with it, but they are wrong, too. Is it possible the timezone offset is "backwards" or something?

Also, should the date and time formatting not respect the Craft localization? For example, if I switch my language to Russian, it shows up the same (with am/pm etc), but in Craft Entries it switches to 05.01.2024, 13:09. It isn't a massive deal, but my assumption was that with the recent changes you made, this was the intention? 🙂

@aaronlinley
Copy link
Contributor Author

Yeah, I can see some issues with the localization. It's not quite working the way I thought it would when using the date-fns formatting that @gustavs-gutmanis mentioned. I need to look into it a little further

@kjmartens
Copy link
Contributor

Yeah, I can see some issues with the localization. It's not quite working the way I thought it would when using the date-fns formatting that @gustavs-gutmanis mentioned. I need to look into it a little further

@gustavs-gutmanis can you meet with @aaronlinley to let him know which adjustments are necessary to resolve this? 🙂

@gustavs-gutmanis
Copy link
Contributor

I can see date-fns doesn't deal with timezones. If you want timezones, we must install the date-fns-tz package, but even that won't solve this, because it doesn't infer the timezone from the browser, like Date does.

You could try using the Intl.DateTimeFormat to achieve this.

Or as an alternative - use Craft's date localization methods to send a parsed date string from the server already.

@aaronlinley aaronlinley requested a review from kjmartens January 9, 2024 16:25
@aaronlinley
Copy link
Contributor Author

@kjmartens @gustavs-gutmanis I've updated the datetime handling to be more reliable now. It sends a ISO string from the server to the frontend, and uses date-fns-tz to format it in the current user's timezone. This displays the date correctly in the current timezone, and updates the value when the timezone is changed via Craft settings (e.g. GMT+1 displays the time as 1 hour earlier than GMT).

Copy link
Contributor

@kjmartens kjmartens left a comment

Choose a reason for hiding this comment

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

Hmm, I am still seeing no difference.

  • Times are still +6hrs ahead for me (e.g. should say 11:00am for me, but says 5:00pm). My timezone is GMT-6.
  • When I switch to a European locale for my user, dates/times are not formatted any differently (e.g. uses 12hr clock instead of 24hr clock).
  • I am getting a new issue... about 30% of the time, a manual page refresh in the builder doesn't show the builder and console logs the following errors:
vendor.js?v=1704819269:2 TypeError: Cannot read properties of undefined (reading 'created')
    at t.SettingsOwnership (client.js?v=1704819269:1:259266)

vendor.js?v=1704819269:2 Uncaught TypeError: Cannot read properties of undefined (reading 'created')
    at t.SettingsOwnership (client.js?v=1704819269:1:259266)

The part of the JS file it refers to is:
c.utcToZonedTime)(new Date(e.created.datetime), t)

@aaronlinley aaronlinley requested a review from kjmartens January 11, 2024 11:46
Copy link
Contributor

@gustavs-gutmanis gustavs-gutmanis left a comment

Choose a reason for hiding this comment

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

Looking good! 👍🏻

Copy link
Contributor

@kjmartens kjmartens left a comment

Choose a reason for hiding this comment

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

Wonderful

@kjmartens kjmartens merged commit 9038427 into v5 Jan 11, 2024
4 checks passed
@kjmartens kjmartens deleted the feat/SFT-604 branch January 11, 2024 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants