Skip to content

Localize all timestamps#21440

Merged
lunny merged 14 commits into
go-gitea:mainfrom
yardenshoham:dates
Oct 17, 2022
Merged

Localize all timestamps#21440
lunny merged 14 commits into
go-gitea:mainfrom
yardenshoham:dates

Conversation

@yardenshoham

Copy link
Copy Markdown
Member

Following

We are now able to localize all timestamps. Some examples:

short-date format, French, user profile page:
image

date-time format, Portuguese, mirror repository settings page:
image

Signed-off-by: Yarden Shoham hrsi88@gmail.com

@yardenshoham yardenshoham marked this pull request as draft October 13, 2022 14:24
@yardenshoham yardenshoham marked this pull request as ready for review October 13, 2022 15:06
@silverwind

Copy link
Copy Markdown
Member

We may be able to do this on backend with #21443. I think it's certainly better to do it there to avoid content flashes. If we want to keep date formatting in frontend, there is the option of #21429 which I devised.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 13, 2022
@yardenshoham

Copy link
Copy Markdown
Member Author

I read both PRs you linked. About the following comment:

    `requestAnimationFrame` is the proper way to mutate DOM without flicker. It can and should be avoided. It's just sloppiness on GitHub's side that they still have these flickers.

    I will also move all stuff in [formatting.js](https://github.com/yardenshoham/gitea/blob/main/web_src/js/features/formatting.js) into this script as well. These are all critical replacements.

Originally posted by @silverwind in #21429 (comment)

If you are still doing that then it's worth merging this as it's simply building on

Any solution of a different implementation would still have to include this JS so I suggest merging this for now

#21410 did this for the activity page, this just does it for all other timestamps, easy win

@silverwind

Copy link
Copy Markdown
Member

If we decide for the JS solution, we can certainly go ahead with this already.

@yardenshoham yardenshoham force-pushed the dates branch 4 times, most recently from 6cd0fff to 24469d9 Compare October 14, 2022 19:15
@yardenshoham

Copy link
Copy Markdown
Member Author

@wxiaoguang wxiaoguang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I can vote LGTM

Comment thread web_src/js/features/formatting.js Outdated
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Oct 15, 2022
@yardenshoham yardenshoham force-pushed the dates branch 2 times, most recently from d9647d2 to 9e14b51 Compare October 15, 2022 11:57
Signed-off-by: Yarden Shoham <hrsi88@gmail.com>
Signed-off-by: Yarden Shoham <hrsi88@gmail.com>
@Gusted Gusted added this to the 1.18.0 milestone Oct 15, 2022
@Gusted Gusted added the type/enhancement An improvement of existing functionality label Oct 15, 2022
Comment thread web_src/js/features/formatting.js Outdated
Comment thread web_src/js/features/formatting.js Outdated
Comment thread web_src/js/features/formatting.js Outdated
yardenshoham and others added 4 commits October 15, 2022 22:44
Co-authored-by: Gusted <williamzijl7@hotmail.com>
Co-authored-by: Gusted <williamzijl7@hotmail.com>
Co-authored-by: Gusted <williamzijl7@hotmail.com>

@Gusted Gusted left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Alright LGTM. Given the other PR to make the JS execution faster, the concern of FOUC shouldn't be that big, if the other PR is merged before 1.18

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Oct 15, 2022
Comment thread templates/admin/auth/list.tmpl
@lunny

lunny commented Oct 17, 2022

Copy link
Copy Markdown
Member

make L-G-T-M work

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/enhancement An improvement of existing functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants