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

link to file from its history #27354

Merged
merged 10 commits into from
Oct 2, 2023
Merged

Conversation

denyskon
Copy link
Member

@denyskon denyskon commented Sep 29, 2023

Fixes #3852
Fixes #26707

Add a button on file history which directs you to the file at the selected commit.

grafik

Co-authored-by: silverwind me@silverwind.io

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Sep 29, 2023
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Sep 29, 2023
@github-actions github-actions bot added the topic/ui Change the appearance of the Gitea UI label Sep 29, 2023
@denyskon denyskon added the type/enhancement An improvement of existing functionality label Sep 29, 2023
@denyskon denyskon added this to the 1.22.0 milestone Sep 29, 2023
@silverwind
Copy link
Member

Also fixes #26707 which might be duplicate.

@silverwind
Copy link
Member

Can we label the column "Actions" like I did in #26998?

Co-authored-by: silverwind <me@silverwind.io>
@denyskon
Copy link
Member Author

@silverwind I applied the suggested changes

@silverwind
Copy link
Member

silverwind commented Sep 29, 2023

Can we move the "Copy SHA" button to Operations as well? Same styling and all. Wrap it in a <div class="gt-df gt-gap-3>.

@pull-request-size pull-request-size bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 29, 2023
@denyskon
Copy link
Member Author

Done
grafik

@silverwind
Copy link
Member

silverwind commented Sep 29, 2023

Did some tweaks:

Screenshot 2023-09-29 at 21 42 20

<th class="two wide right aligned">{{ctx.Locale.Tr "repo.commits.date"}}</th>
<th class="one wide right aligned">{{ctx.Locale.Tr "repo.commit.operations"}}</th>
Copy link
Member

Choose a reason for hiding this comment

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

Sounds a bit weird to waste so much space simply because of the column title (that isn't even particularly helpful).
What about leaving it empty instead?

Suggested change
<th class="one wide right aligned">{{ctx.Locale.Tr "repo.commit.operations"}}</th>
<th class="one wide right aligned"></th>

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking about getting rid of the whole table header. For example, branch list is without header as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is this tiny PR just being blown up to a full commit list redesign? 😆

Copy link
Member

Choose a reason for hiding this comment

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

I think the first action is simply not including this column header.
The rest can be left for other PRs.

Copy link
Member

Choose a reason for hiding this comment

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

I guess it looks alright even without the header here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think in the future we should get rid of the HTML table completely. For now - I'm fine with leaving the title empty.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, looks acceptable:

Screenshot 2023-09-29 at 21 52 42

@silverwind
Copy link
Member

The "View File" name is inaccurate when viewing a directory's history. Maybe we should use the same or similar string that GitHub does? "View at this point in the history", I would omit the the in the string.

@denyskon
Copy link
Member Author

@silverwind Where did you push your changes? 😆

Did some tweaks:
Screenshot 2023-09-29 at 21 42 20

@delvh
Copy link
Member

delvh commented Sep 30, 2023

Could you please update the screenshot in the PR description?

@denyskon
Copy link
Member Author

I'd wait for the tweaks by silverwind as they change the appearance a bit

@@ -76,6 +76,10 @@
{{else}}
<td class="text right aligned">{{TimeSince .Author.When ctx.Locale}}</td>
{{end}}
<td class="text right aligned">
<button class="btn interact-bg" data-clipboard-text="{{.ID}}">{{svg "octicon-copy"}}</button>
{{if $.FileName}}<a class="btn interact-bg" data-tooltip-content="{{ctx.Locale.Tr "repo.diff.view_file"}}" href="{{printf "%s/src/commit/%s/%s" $commitRepoLink (PathEscape .ID.String) $.FileName}}">{{svg "octicon-file-code"}}</a>{{end}}
Copy link
Member

@puni9869 puni9869 Sep 30, 2023

Choose a reason for hiding this comment

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

Suggested change
{{if $.FileName}}<a class="btn interact-bg" data-tooltip-content="{{ctx.Locale.Tr "repo.diff.view_file"}}" href="{{printf "%s/src/commit/%s/%s" $commitRepoLink (PathEscape .ID.String) $.FileName}}">{{svg "octicon-file-code"}}</a>{{end}}
{{if $.FileName}}<a class="btn interact-bg" data-tooltip-content="{{ctx.Locale.Tr "repo.diff.view_file"}}" href="{{printf "%s/src/commit/%s/%s" $commitRepoLink (PathEscape .ID.String) $.FileName}}">{{svg "octicon-file-code"}}</a>{{end}}

could you shift this to backend {{printf "%s/src/commit/%s/%s" $commitRepoLink (PathEscape .ID.String) $.FileName}}, we don't want regression.

@puni9869
Copy link
Member

Date column header can be left aligned.

@silverwind
Copy link
Member

silverwind commented Sep 30, 2023

Ah it seems it did not push, I was getting "remote rejected", will check.

@silverwind
Copy link
Member

silverwind commented Sep 30, 2023

Seems I still can not push (is collaboration enabled on this PR?):

 ! [remote rejected]       denyskon/commit-file-link -> denyskon/commit-file-link (permission denied)

Here is the patch:

diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini
index 8667e77a89..ec37d64fec 100644
--- a/options/locale/locale_en-US.ini
+++ b/options/locale/locale_en-US.ini
@@ -96,8 +96,9 @@ disabled = Disabled
 locked = Locked

 copy = Copy
 copy_url = Copy URL
+copy_hash = Copy hash
 copy_content = Copy content
 copy_branch = Copy branch name
 copy_success = Copied!
 copy_error = Copy failed
diff --git a/templates/repo/commits_list.tmpl b/templates/repo/commits_list.tmpl
index c5003af4ae..db658de3e7 100644
--- a/templates/repo/commits_list.tmpl
+++ b/templates/repo/commits_list.tmpl
@@ -75,11 +75,13 @@
 							<td class="text right aligned">{{TimeSince .Committer.When ctx.Locale}}</td>
 						{{else}}
 							<td class="text right aligned">{{TimeSince .Author.When ctx.Locale}}</td>
 						{{end}}
-						<td class="text right aligned">
-							<button class="btn interact-bg" data-clipboard-text="{{.ID}}">{{svg "octicon-copy"}}</button>
-							{{if $.FileName}}<a class="btn interact-bg" data-tooltip-content="{{ctx.Locale.Tr "repo.diff.view_file"}}" href="{{printf "%s/src/commit/%s/%s" $commitRepoLink (PathEscape .ID.String) $.FileName}}">{{svg "octicon-file-code"}}</a>{{end}}
+						<td class="gt-pt-0 gt-pb-0">
+							<div class="gt-df gt-je">
+								<button class="btn interact-bg gt-p-3" data-tooltip-content="{{ctx.Locale.Tr "copy_hash"}}" data-clipboard-text="{{.ID}}">{{svg "octicon-copy"}}</button>
+								{{if $.FileName}}<a class="btn interact-bg gt-p-3" data-tooltip-content="{{ctx.Locale.Tr "repo.diff.view_file"}}" href="{{printf "%s/src/commit/%s/%s" $commitRepoLink (PathEscape .ID.String) $.FileName}}">{{svg "octicon-file-code"}}</a>{{end}}
+							</div>
 						</td>
 					</tr>
 				{{end}}
 			</tbody>

@wxiaoguang
Copy link
Contributor

I like <td class="text right aligned"> more than the massive gt helpers.

@denyskon
Copy link
Member Author

denyskon commented Oct 1, 2023

@delvh @silverwind I pushed the changes and updated the screenshot.

@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 1, 2023
@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 1, 2023
@techknowlogick techknowlogick enabled auto-merge (squash) October 2, 2023 03:16
@techknowlogick techknowlogick added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Oct 2, 2023
@techknowlogick techknowlogick merged commit 33de64c into go-gitea:main Oct 2, 2023
26 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Oct 2, 2023
@denyskon denyskon deleted the commit-file-link branch October 2, 2023 05:21
@silverwind
Copy link
Member

I like <td class="text right aligned"> more than the massive gt helpers.

Need the flexbox to vertically center and for the gap. Text-based layout is not suitable for these tasks.

@@ -1283,6 +1284,7 @@ commits.signed_by_untrusted_user = Signed by untrusted user
commits.signed_by_untrusted_user_unmatched = Signed by untrusted user who does not match committer
commits.gpg_key_id = GPG Key ID
commits.ssh_key_fingerprint = SSH Key Fingerprint
commits.view_path=View at this point in history
Copy link
Member

Choose a reason for hiding this comment

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

If there is a ini linter, I would make it lint for spaces around = 😆

<td class="gt-pt-0 gt-pb-0">
<div class="gt-df gt-je">
<button class="btn interact-bg gt-p-3" data-tooltip-content="{{ctx.Locale.Tr "copy_hash"}}" data-clipboard-text="{{.ID}}">{{svg "octicon-copy"}}</button>
{{if $.FileName}}<a class="btn interact-bg gt-p-3" data-tooltip-content="{{ctx.Locale.Tr "repo.commits.view_path"}}" href="{{printf "%s/src/commit/%s/%s" $commitRepoLink (PathEscape .ID.String) $.FileName}}">{{svg "octicon-file-code"}}</a>{{end}}
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 $.FileName needs to be escaped to URL parameter. Otherwise C++ or C# breaks the URL.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, likely PathEscape.

Copy link
Contributor

Choose a reason for hiding this comment

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

-> Improve file history UI and fix URL escaping bug #27531

lunny pushed a commit that referenced this pull request Oct 9, 2023
Follow #27354

Major changes:

1. The `right aligned` in `<th class="one wide right aligned">` is a
no-op because it doesn't have any content
2. The `gt-df` in `<td class="sha gt-df">` was wrong, it causes UI
misalignment, a table cell shouldn't be "flex"
3. Use `gt-py-0` for `gt-pt-0 gt-pb-0`
4. Simplify the layout for buttons, because the `text right aligned` is
widely used and good enough, it doesn't make sense to introduce the
`<div class="gt-df gt-je">`
5. Escape the `$.FileName` correctly


Before:


![image](https://github.com/go-gitea/gitea/assets/2114189/eb2ced3f-1dad-4149-9ed2-aee4c0663621)

After:


![image](https://github.com/go-gitea/gitea/assets/2114189/08244b61-416b-4279-b495-029bc0a96f67)
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Dec 31, 2023
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. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. topic/ui Change the appearance of the Gitea UI type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Button in file commit history to view file at commit Link to file on specific commit from its history
7 participants