Skip to content

Remove untranslatable on_date key #24106

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.

Sign up for GitHub

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

Merged
merged 16 commits into from
Apr 15, 2023
Merged

Remove untranslatable on_date key #24106

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
4785128
Add ability to tune the relative-time elements in `TimeSince` functions
yardenshoham Apr 13, 2023
a54bcb0
Avoid escaping quotes
yardenshoham Apr 13, 2023
a8b06c6
Merge branch 'main' into tense-past
yardenshoham Apr 13, 2023
fd4e20b
Fix quotes
yardenshoham Apr 13, 2023
eab3049
Merge branch 'main' into tense-past
yardenshoham Apr 14, 2023
9955b2c
improve
wxiaoguang Apr 14, 2023
f746425
Drop `duration`
yardenshoham Apr 14, 2023
f949758
Remove `on_date`
yardenshoham Apr 14, 2023
24a4039
remove untranslatable word
wxiaoguang Apr 14, 2023
b130c9e
Format package view date
yardenshoham Apr 14, 2023
daecc08
Fix render
yardenshoham Apr 14, 2023
5bf5d41
fmt
yardenshoham Apr 14, 2023
d8e73a5
fix lint
wxiaoguang Apr 14, 2023
2a4fe6e
Revert "tense=auto support"
wxiaoguang Apr 14, 2023
46226d3
Merge branch 'main' into tense-past
yardenshoham Apr 14, 2023
d0df687
Merge branch 'main' into tense-past
yardenshoham Apr 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions modules/timeutil/since.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,13 +115,13 @@ func timeSincePro(then, now time.Time, lang translation.Locale) string {
}

// TimeSince renders relative time HTML given a time.Time
func TimeSince(then time.Time, lang translation.Locale) template.HTML {
func TimeSince(then time.Time, lang translation.Locale, relativeTimeOptions ...string) template.HTML {
timestamp := then.UTC().Format(time.RFC3339)
// declare data-tooltip-content attribute to switch from "title" tooltip to "tippy" tooltip
return template.HTML(fmt.Sprintf(`<relative-time class="time-since" prefix="%s" datetime="%s" data-tooltip-content data-tooltip-interactive="true">%s</relative-time>`, lang.Tr("on_date"), timestamp, timestamp))
return template.HTML(fmt.Sprintf(`<relative-time class="time-since" %s prefix="%s" datetime="%s" data-tooltip-content data-tooltip-interactive="true">%s</relative-time>`, strings.Join(relativeTimeOptions, " "), lang.Tr("on_date"), timestamp, timestamp))
Copy link
Contributor

Choose a reason for hiding this comment

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

We should remove this on_date. It's not translatable.

TimeSince should only be used inside other translatable strings, let other strings have proper wording

Copy link
Contributor

@wxiaoguang wxiaoguang Apr 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Even in English, I do not think it is fine.

In the list, it doesn't need the on word:

image

Copy link
Member Author

Choose a reason for hiding this comment

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

Unrelated to this PR, #24074 is tracking this

Copy link
Member Author

Choose a reason for hiding this comment

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

After this PR this on in your screenshot will disappear

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is related .... it really doesn't look right.

In old code, there is no on prefix of the TimeSince output either, this on prefix was just added recently, so I do not think it is necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

the implications are not

Hmm .... in history , IIRC the TimeSince always used relative date, then <relative-time> changed the behavior.

Now, we just make <relative-time> always use tense=past, then it behave like the old TimeSince , then everything should be fine, and just like before.

Correct me if I am wrong.

Copy link
Member Author

Choose a reason for hiding this comment

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

If the date was in the future, it probably rendered in X days. With tense=past it will render now. That's what I'm worried about...

BTW feel free to push changes here, I won't block (you have better historical context here then me)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, the on <date> format is new with the element. We could for now opt to always set past tense and remove the on translation to restore old behaviour, but I do prefer the on format for far-past dates as well, but it needs to be worked out first how to correctly translate that format (#24074).

Copy link
Contributor

Choose a reason for hiding this comment

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

feel free to push changes here,

Thank you very much for the trust, I will try it myself to see whether I really understand the problem (see you in a few hours) 😄

Copy link
Member

Choose a reason for hiding this comment

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

If the date was in the future, it probably rendered in X days. With tense=past it will render now. That's what I'm worried about...

BTW feel free to push changes here, I won't block (you have better historical context here then me)

That's no problem, GH also did that.

}

// TimeSinceUnix renders relative time HTML given a TimeStamp
func TimeSinceUnix(then TimeStamp, lang translation.Locale) template.HTML {
return TimeSince(then.AsLocalTime(), lang)
func TimeSinceUnix(then TimeStamp, lang translation.Locale, relativeTimeOptions ...string) template.HTML {
return TimeSince(then.AsLocalTime(), lang, relativeTimeOptions...)
}
4 changes: 2 additions & 2 deletions templates/repo/view_list.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
</span>
{{end}}
</th>
<th class="text grey right age">{{if .LatestCommit}}{{if .LatestCommit.Committer}}{{TimeSince .LatestCommit.Committer.When $.locale}}{{end}}{{end}}</th>
<th class="text grey right age">{{if .LatestCommit}}{{if .LatestCommit.Committer}}{{TimeSince .LatestCommit.Committer.When $.locale "tense=past"}}{{end}}{{end}}</th>
</tr>
</thead>
<tbody>
Expand Down Expand Up @@ -87,7 +87,7 @@
{{end}}
</span>
</td>
<td class="text right age three wide">{{if $commit}}{{TimeSince $commit.Committer.When $.locale}}{{end}}</td>
<td class="text right age three wide">{{if $commit}}{{TimeSince $commit.Committer.When $.locale "tense=past"}}{{end}}</td>
</tr>
{{end}}
</tbody>
Expand Down