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.

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

Jump to bottom

Show source/target branches on PR's list #19747

Merged
merged 16 commits into from
May 25, 2022
Merged

Show source/target branches on PR's list #19747

merged 16 commits into from
May 25, 2022

Conversation

IT-AlexKor
Copy link
Contributor

@IT-AlexKor IT-AlexKor commented May 18, 2022 •
edited by lafriks

Add ability to show source/target branches for Pull Request's list. It can be useful to see which branches are used in each PR right in the list.

Implemented in this PR:

  • Ability to enable/disable branch info displaying on PR's list
  • Light and dark theme support
  • Long names truncation (for repo and branch names separately)
  • Optional repo displaying (only if repo different than PR's repo)

Examples:
image
image

@IT-AlexKor IT-AlexKor mentioned this pull request May 18, 2022
Closed
@6543 6543 added the topic/ui Change the appearance of the Gitea UI label May 18, 2022
@6543
Copy link
Member

6543 commented May 18, 2022

I wonder if we could hide it for mobile/smal-screen view?

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 18, 2022
@IT-AlexKor
Copy link
Contributor Author

I can try. Can you give me any example from existing code (how to detect mobile device)?

lunny
lunny reviewed May 18, 2022
View reviewed changes
modules/setting/setting.go Outdated Show resolved Hide resolved
@lunny
Copy link
Member

lunny commented May 18, 2022

And I think we can display them like what they displayed in the pull request view page.

@IT-AlexKor
Copy link
Contributor Author

IT-AlexKor commented May 18, 2022 •
edited

we can display them like what they displayed in the pull request view page

I'm not sure - the interface will be too complex:
image

and don't forget about additional blocks like
image

@IT-AlexKor
Copy link
Contributor Author

As reference I use GitHub representation
image

@6543 6543 added this to the 1.17.0 milestone May 20, 2022
@6543 6543 added the type/enhancement An improvement of existing functionality label May 20, 2022
@lafriks
Copy link
Member

lafriks commented May 24, 2022

Do we really need option to disable this in settings? We have way too many options already and this makes it harder to maintain all their combinations :)

@IT-AlexKor
Copy link
Contributor Author

I don't know :) In my production these option is always enabled. I can remove it if we decide so.

@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 May 24, 2022
@6543
Copy link
Member

6543 commented May 24, 2022

I'm ok with it as is

@zeripath
Copy link
Contributor

I agree with @lafriks here - people can simply edit the template to avoid putting this in if they really don't want it.

@lafriks
Copy link
Member

lafriks commented May 24, 2022

Or hide with css. I would understand if that was some kind of per-user setting where he could show/hide this but as displaying or not does not affect server performance I don't see need for such options in app.ini

@wxiaoguang
Copy link
Contributor

+1 for not introducing more setting options.

@IT-AlexKor
Copy link
Contributor Author

@lafriks @zeripath I removed the option from config

@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 May 25, 2022
wxiaoguang
wxiaoguang requested changes May 25, 2022
View reviewed changes
Copy link
Contributor

@wxiaoguang wxiaoguang left a comment •
edited

Choose a reason for hiding this comment

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

Need more reviews and fine tune ....

(since there are already 2 L-G-T-M, put a request change here to prevent from accident merge)

templates/shared/issuelist.tmpl Outdated Show resolved Hide resolved
wxiaoguang
wxiaoguang reviewed May 25, 2022
View reviewed changes
web_src/less/_base.less Outdated Show resolved Hide resolved
@alex-kor-wrk
PR go-gitea#19747
25196de 
* Add AppSubURL to URL's
* Synchronize coloring schemes
* Fix SVG size
@wxiaoguang
refactor issue list branch names
0afa9db 
* clarify getRepoIDs usage, now it also returns the repo IDs for PullRequest.HeadRepo
* remove unnecessary ctx.Data["AppSubURL"], use `repo.HTMLURL` instead
* use span inside a tag, instead of div
* escape the branch names in URL
* remove unnecessary CSS styles and variables
* use character instead of SVG arrows
@wxiaoguang
Copy link
Contributor

wxiaoguang commented May 25, 2022 •
edited

Thank you for your updating, it looks fine. I did some refactoring on it, major changes and reasons:

  • clarify getRepoIDs usage, now it also returns the repo IDs for PullRequest.HeadRepo
  • remove unnecessary ctx.Data["AppSubURL"], use repo.HTMLURL instead this is a must
  • use span inside the a tag, instead of div (well, usually a div shouldn't appear inside the a tag)
  • use predefined bold class for a tag
  • escape the branch names in URL this is a must
  • remove unnecessary CSS styles and variables
  • use character « (aka « )instead of SVG arrows (I think the character would save a little rendering resource of browser, if you like SVG, it's also fine, I left the SVG code in comment, feel free to change it 😊)

image

@IT-AlexKor
Copy link
Contributor Author

@wxiaoguang looks great. Thanks for help

@lafriks lafriks added the status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR label May 25, 2022
@lafriks
Copy link
Member

lafriks commented May 25, 2022 •
edited

blocking until either one or other is left just no to leave commented out code in final merge

@wxiaoguang @lafriks
Update templates/shared/issuelist.tmpl
d0c27d7 
Co-authored-by: Lauris BH <lauris@nix.lv>
@lafriks lafriks removed the status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR label May 25, 2022
@codecov-commenter

This comment was marked as off-topic.

Sign in to view
@6543
Copy link
Member

6543 commented May 25, 2022

🚀

@6543 6543 merged commit 0692f43 into go-gitea:main May 25, 2022
zjjhot added a commit to zjjhot/gitea that referenced this pull request May 26, 2022
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
7409881 
* giteaofficial/main:
  Make WIP prefixes case insensitive, e.g. allow `Draft` as a WIP prefix (go-gitea#19780)
  Fix follower display on user page (go-gitea#19805)
  Show source/target branches on PR's list (go-gitea#19747)
@IT-AlexKor IT-AlexKor mentioned this pull request Aug 23, 2022
Closed
AbdulrhmnGhanem pushed a commit to kitspace/gitea that referenced this pull request Aug 24, 2022
@IT-AlexKor @alex-kor-wrk
@wxiaoguang @lafriks @AbdulrhmnGhanem
Show source/target branches on PR's list (go-gitea#19747)
e820694 
Add ability to show source/target branches for Pull Request's list. It can be useful to see which branches are used in each PR right in the list.

Co-authored-by: Alexey Korobkov <akorobkov@cian.ru>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
Co-authored-by: Lauris BH <lauris@nix.lv>
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lunny lunny lunny left review comments

@lafriks lafriks lafriks approved these changes

@wxiaoguang wxiaoguang wxiaoguang approved these changes

@6543 6543 6543 approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/ui Change the appearance of the Gitea UI type/enhancement An improvement of existing functionality
Projects
None yet
Milestone
1.17.0
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@IT-AlexKor @6543 @lunny @lafriks @zeripath @wxiaoguang @codecov-commenter @GiteaBot @alex-kor-wrk