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

Show branches and tags that contain a commit #25180

Merged
merged 45 commits into from Jul 27, 2023

Conversation

delvh
Copy link
Member

@delvh delvh commented Jun 9, 2023

Now, you can see for a commit which existing branches and tags contain it.
You first have to click on the load branches and tags button, they are not preloaded by default.
All branches and tags are ordered descending by creation date.
You can even see without much hassle if the given commit is already part of the default branch.

Closes #25152

Screenshots

Initial

image

Loaded

image

@delvh delvh added type/feature Completely new functionality. Can only be merged if feature freeze is not active. topic/ui Change the appearance of the Gitea UI labels Jun 9, 2023
@delvh delvh self-assigned this Jun 9, 2023
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 9, 2023
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 9, 2023
@delvh delvh changed the title Load referencing branches and tags of a commit Show branches and tags that contain a commit Jun 9, 2023
Comment on lines 435 to 443
// LoadBranchName load branch name for commit
func (c *Commit) LoadBranchName() (err error) {
if len(c.Branch) != 0 {
return
}

c.Branch, err = c.GetBranchName()
return err
}
Copy link
Member Author

@delvh delvh Jun 9, 2023

Choose a reason for hiding this comment

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

Note regarding the changes (above this line) in this file:
They are unused already, hence I removed them.
Below this line was made redundant through this PR.

@delvh delvh mentioned this pull request Jun 9, 2023
@silverwind
Copy link
Member

For the branches, I guess a ... expander button besides the main branch with a tooltip will do. For tags, I'm not sure. Maybe also a inline "Show Tags" button.

I would put all branches and tags into the same wrapping flexbox please and add a icon to each.

@delvh
Copy link
Member Author

delvh commented Jun 9, 2023

I don't think it's a good idea to load them separately.
They are loaded at the same time.

@silverwind
Copy link
Member

Or maybe just two buttons besides the main branch: "Show all branches", "Show all tags" that expand to the inline list of branches and tags.

@delvh
Copy link
Member Author

delvh commented Jun 9, 2023

Furthermore, I've even contemplated removing the main branch you mentioned but decided against it to keep this PR simple.

@silverwind
Copy link
Member

Okay if you load them together, that's even simpler. Just a ... button with "Show all branches and tags containing this commit" that expands them all inline.

@silverwind
Copy link
Member

silverwind commented Jun 9, 2023

Why not do what GitHub does and show a tag range of newest to oldest with an expander in between? It gives just enough info that most of the time I do not need to click expand at all:

Screenshot 2023-06-09 at 21 33 29

@delvh
Copy link
Member Author

delvh commented Jun 9, 2023

Tags could have a similar range mechanism as there, I'm mostly also only interested in latest and oldest tag for a commit

Well… I've explicitly decided against such a mechanism.
We can either get the full information or none.
I've decided for the current implementation to only load the information when the button is pressed as that makes a lot more sense from a user perspective:
you only get the information you requested speeding up most page loads, and we don't have to find ways to hack around the UI to get the expected outcome without cluttering up the screen for everyone else. All that for the price of a single click you have to do either way.
Otherwise this PR is really going to get messy.

I mean, if you want to try your luck feel free to, but I think the current mechanism suffices for now.

(
And a word of advice: I'm so abrasive here as your proposal requires effectively rewriting the whole PR. I've taken care to make the code as reusable as possible but I cannot bend physics. I've decided in the beginning to load the information asynchronously, and decided to implement in that direction.
So, in the end it comes down to is the time worth the benefit?:
The functionality is already there and has an intuitive UI, so I don't think wasting a few hours is worth it for such a small benefit.
)

@silverwind
Copy link
Member

silverwind commented Jun 9, 2023

I guess my ideal UI would be

[branchicon] [main] (pr#) ...
[tagicon] [tag3] ... [tag1]

Where both expanders load the stuff individually. Can we at least decouple the two loads?

@delvh
Copy link
Member Author

delvh commented Jun 9, 2023

As I said: Feel free to implement, but in general I don't quite see the usefulness behind requiring two clicks instead of one.

@KN4CK3R
Copy link
Member

KN4CK3R commented Jun 12, 2023

I see it like delvh.

Two suggestions:

  1. Could you add more spacing so the tags don't appear in the icon area?
    grafik
  2. Could you make the branches/tags clickable like on Github?

@wxiaoguang
Copy link
Contributor

Made some changes. The UI doesn't change much:

image

Copy link
Member Author

@delvh delvh left a comment

Choose a reason for hiding this comment

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

I completely agree with your changes.

(But it is funny that you reverted the layout basically to what it was before the PR review started 😁)

modules/context/context_response.go Show resolved Hide resolved
web_src/css/helpers.css Outdated Show resolved Hide resolved
web_src/js/index.js Outdated Show resolved Hide resolved
@wxiaoguang
Copy link
Contributor

(But it is funny that you reverted the layout basically to what it was before the PR review started 😁)

Hmm, I didn't realize that it was asked to do that. I didn't make the "icon" alone because the spaces above/below it look very strange to me. So I make the icon "inline" with other texts.

But, if most people like the "icon with vertical spaces", I can try to "revert the revert".

@delvh
Copy link
Member Author

delvh commented Jul 26, 2023

At least as far as I'm concerned, I can live with both.
It just so happens that this was also pretty much my first approach (except for the SVG bug).
I don't have a clear preference.

@wxiaoguang
Copy link
Contributor

To revert, it could look like this (ps: I didn't use vertical middle alignment because the space above the icon looks strange to me ....)

c573def

image

@wxiaoguang wxiaoguang added this to the 1.21.0 milestone Jul 26, 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 Jul 26, 2023
@delvh delvh added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jul 26, 2023
@lunny
Copy link
Member

lunny commented Jul 27, 2023

Interesting, I only noticed your message randomly now.

Regarding the not contained anywhere: How did you achieve that? Force pushing the commit out of the history? I mean, I can add a nowhere or something like that when a commit is neither contained in a branch or a tag.

Regarding the opposite case: No, I don't think we should handle that case. It's pytorch's fault if they misuse their branches like that. Their approach already seems to be quite bad when you want to see what branches are available at all. I don't see why we should handle that edge case separately. It simply complicates the logic and serves no real purpose, since the user requested to see all branches that contain this commit.

I'm OK for the second and for the first one, the commits maybe garbage commits but not destroy yet.

@delvh delvh merged commit bd6ef71 into go-gitea:main Jul 27, 2023
24 checks passed
@delvh delvh deleted the feature/load-referencing-branches-and-tags branch July 27, 2023 10:47
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jul 27, 2023
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jul 28, 2023
* giteaofficial/main: (21 commits)
  improve unit test for caching (go-gitea#26185)
  Render plaintext task list items for markdown files (go-gitea#26186)
  Add tooltip to describe LFS table column and color `delete LFS file` button red (go-gitea#26181)
  Show branches and tags that contain a commit (go-gitea#25180)
  Release attachments duplicated check (go-gitea#26176)
  Calculate MAX_WORKERS default value by CPU number (go-gitea#26177)
  Fixing redirection issue for logged-in users (go-gitea#26105)
  Update govulncheck, fix typo (go-gitea#26168)
  Fix handling of plenty Nuget package versions (go-gitea#26075)
  Fix typos in Contributing.md (go-gitea#26170)
  Disable download action logs button when there's no logs (go-gitea#26114)
  Re-add static images to docs (go-gitea#26167)
  Update email-setup.en-us.md (go-gitea#26068)
  Improve display of Labels/Projects/Assignees sort options (go-gitea#25886)
  Fix wrong branch name in rename branch modal (go-gitea#26146)
  Doc update swagger doc for POST /orgs/{org}/teams  (go-gitea#26155)
  Fix UI regression of asciinema player (go-gitea#26159)
  refactor improve NoBetterThan (go-gitea#26126)
  Update Chinese documents (go-gitea#26139)
  Fix bugs in LFS meta garbage collection (go-gitea#26122)
  ...
silverwind added a commit to silverwind/gitea that referenced this pull request Jul 30, 2023
* origin/main: (43 commits)
  Add `/public/assets` to `.ignore` (go-gitea#26232)
  Fix attachment clipboard copy on insecure origin (go-gitea#26224)
  Fix commit compare style (go-gitea#26209)
  Fix unable to display individual-level project (go-gitea#26198)
  Fix access check for org-level project (go-gitea#26182)
  Fixed incorrect locale references (go-gitea#26218)
  Use calendar icon for `Joined on...` in profiles (go-gitea#26215)
  Add changelog for 1.20.2 (go-gitea#26208)
  Add commits dropdown in PR files view and allow commit by commit review (go-gitea#25528)
  Warn instead of reporting an error when a webhook cannot be found (go-gitea#26039)
  Fixing the align of commit stats in commit_page template. (go-gitea#26161)
  Fix allowed user types setting problem (go-gitea#26200)
  Hide branch/tag icon if branches/tags are empty (go-gitea#26204)
  Prevent primary key update on migration (go-gitea#26192)
  improve unit test for caching (go-gitea#26185)
  Render plaintext task list items for markdown files (go-gitea#26186)
  Add tooltip to describe LFS table column and color `delete LFS file` button red (go-gitea#26181)
  Show branches and tags that contain a commit (go-gitea#25180)
  Release attachments duplicated check (go-gitea#26176)
  Calculate MAX_WORKERS default value by CPU number (go-gitea#26177)
  ...
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Oct 25, 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/L Denotes a PR that changes 100-499 lines, ignoring generated files. topic/ui Change the appearance of the Gitea UI type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show branches and tags containing a commit
6 participants