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

Fix branches presentation on PR's list #20931

Closed
wants to merge 4 commits into from
Closed

Fix branches presentation on PR's list #20931

wants to merge 4 commits into from

Conversation

IT-AlexKor
Copy link
Contributor

This is a hotfix for UI presentation of branches on PR's list (implemented in #19747)

Current representation (before fix)
before_fix

New representation
after_fix

Please include/backport (if possible) to v1.17.2 for example

@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Aug 23, 2022
@zeripath zeripath added topic/ui Change the appearance of the Gitea UI and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Aug 23, 2022
@zeripath zeripath added this to the 1.18.0 milestone Aug 23, 2022
@silverwind
Copy link
Member

Truncation is good, but I don't agree it making it bold again, I think it look better without bold.

@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Aug 23, 2022
silverwind
silverwind reviewed Aug 23, 2022
View reviewed changes
templates/shared/issuelist.tmpl Outdated
{{/* inline to remove the spaces between spans */}}
{{if ne .RepoID .PullRequest.BaseRepoID}}<span class="truncated-name">{{.PullRequest.BaseRepo.OwnerName}}</span>:{{end}}<span class="truncated-name">{{.PullRequest.BaseBranch}}</span>
{{if ne .RepoID .PullRequest.BaseRepoID}}<span class="truncated-name dib vb">{{.PullRequest.BaseRepo.OwnerName}}</span>:{{end}}<span class="truncated-name dib vb">{{.PullRequest.BaseBranch}}</span>
Copy link
Member

Choose a reason for hiding this comment

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

vb does not appear to be a valid helper class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

deleted

silverwind
silverwind requested changes Aug 23, 2022
View reviewed changes
Copy link
Member

@silverwind silverwind left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure how this truncation is supposed to work, maybe mobile layout only? I think generally we want to set max-width and use .ellipsis to limit to a certain length, but unless there is a strong reason to truncate the branch name, I wouldn't do it.

Branch name holds important information and if we can get it to wrap, it would be better than just cutting it off like that.

@IT-AlexKor
Copy link
Contributor Author

@silverwind I will try to explain my decisions.

Why bolded branch names?

  1. Easier to see - without bold it's look like "grey on grey" with a small color difference
  2. Faster to find in a list - one of the scenarios in our teams is to review only PR's that are compared with the master branch only. So when the source/target branches are bolded it's easier to find requested PR's in list by eyes. Without bold whole list looks like a single huige block of text which hard to analyze fastly

Why truncated names?

  1. Each PR in list already have many possible sub-items (num of approvers, num of change request, author, creation date and etc) and when they stick together the line is getting too long and line breaks.
  2. Before Show source/target branches on PR's list #19747 there are no information about source/target branches was present in list, so my decision didn't make it worse than before. Usually there are no truncating becuase branch name is already short.

About "max-width and use .ellipsis": if you know how to make it better please suggest your solution. I have a better experience in backend than in frontend.

@silverwind
Copy link
Member

  1. I don't agree with bold, sorry. We just removed the bold in Make branch icon stand out more #20726 and I think bold should be used very sparingly in UI, this is not a place for it.

  2. .truncated-name is truncating to 10em, which makes even moderate branch names tuncated. I think it's truncating too soon to be any useful:

image

@IT-AlexKor
Copy link
Contributor Author

I didn't know about #20726 and thought that original UI from #19747 was broken accidentally by somebody else.

Sorry for the inconvenience and thanks for your time and detailed explanation. I'm closing current PR (nothing to fix)

@IT-AlexKor IT-AlexKor closed this Aug 25, 2022
@lunny lunny removed this from the 1.18.0 milestone Dec 20, 2022
@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

@silverwind silverwind silverwind requested changes

@zeripath zeripath zeripath approved these changes

Assignees
No one assigned
Labels
lgtm/need 1 This PR needs approval from one additional maintainer to be merged. topic/ui Change the appearance of the Gitea UI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@IT-AlexKor @silverwind @zeripath @lunny @GiteaBot