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

Fix ::User Profile Page - Project/Packages Tabs Have Inconsistent Layout #25108

Conversation

puni9869
Copy link
Member

@puni9869 puni9869 commented Jun 6, 2023

Fix ::User Profile Page Project Tab Have Inconsistent Layout and Style
Added the big_avator for consistency in the all header_items tabs.

#24871

Description

in the user profile page the Packages and Projects tab have small icons for user but other tabs have bigger profile picture with user info:

Screenshots

For Packages And Projects:

image

For Other Tabs:

image

Before

image

After changes

Project View
image

Packages View
image

Org view for projects page

image

Org view for packages page

image

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 6, 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 6, 2023
@puni9869
Copy link
Member Author

puni9869 commented Jun 6, 2023

@silverwind

@silverwind
Copy link
Member

Not sure if related to this PR, but I fixed this issue in e0813ef:

@silverwind
Copy link
Member

silverwind commented Jun 6, 2023

You missed the Code tab which you can see when enabling the repo indexer in app.ini:

[indexer]
REPO_INDEXER_ENABLED = true
REPO_INDEXER_PATH    = indexers/repos.bleve

It still renders full width. Please give it the same treatment.

Screenshot 2023-06-06 at 23 18 04

Other than that, I think this looks good from a quick test.

@pull-request-size pull-request-size bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 7, 2023
@puni9869
Copy link
Member Author

puni9869 commented Jun 7, 2023

Not sure if related to this PR, but I fixed this issue in e0813ef:

This is needed.

@silverwind
Copy link
Member

You mean for the code tab? Yes I could do it there as well. Also, I see I missed the all option in the package selection field, will correct later.

@puni9869
Copy link
Member Author

puni9869 commented Jun 7, 2023

Exactly,
You take it forward for merge and further fix [if any tiniest fix required from your side]. I think Fix ::User Profile Page - Project/Packages Tabs Have Inconsistent Layout is done with the functionality.

Thanks @silverwind for the support.

@silverwind silverwind added type/enhancement An improvement of existing functionality topic/ui Change the appearance of the Gitea UI labels Jun 7, 2023
@silverwind
Copy link
Member

silverwind commented Jun 8, 2023

@puni9869 Some issues:

  1. "Projects" tab is missing while "Code" tab is active:
Screenshot 2023-06-08 at 11 53 57 Screenshot 2023-06-08 at 11 54 04
  1. On "Projects" tab, the tab bar's line is too wide, streching beyond the navbar even:
Screenshot 2023-06-08 at 11 55 11

Can the tab bar template be shared to avoid these and future issues where these templates go out of sync?

Code tab search looks alright and does not flash incorrect content, so needs no adjustment imho.

@silverwind
Copy link
Member

Also, I see I missed the all option in the package selection field, will correct later.

Fixed this in cc79eba.

@puni9869
Copy link
Member Author

puni9869 commented Jun 8, 2023

That headerbar template is shared among all
I'll fix it asap.

Copy link
Member

@silverwind silverwind left a comment

Choose a reason for hiding this comment

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

Appears to work fine now.

@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 Jun 9, 2023
@yekanchi
Copy link

looks good to me 2, thatnks

@puni9869
Copy link
Member Author

Can we get one more approval.

@lunny lunny added this to the 1.21.0 milestone Jun 13, 2023
@@ -19,17 +20,16 @@ const (

// CodeSearch render user/organization code search page
func CodeSearch(ctx *context.Context) {
user.RenderProfileBigAvatar(ctx)
Copy link
Contributor

Choose a reason for hiding this comment

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

The name and usage seems not ideal.

  • The name: it doesn't "render", it only prepares some data
  • If the response has been written in it, the caller shouldn't continue (by checking Written)

Copy link
Member Author

@puni9869 puni9869 Jun 14, 2023

Choose a reason for hiding this comment

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

Correct, and I am even in favour of changing the naming pattern, but when I see https://github.com/go-gitea/gitea/pull/25108/files/6c990c4da74a329f700ffffaa5ddfa1d6195fd7b#diff-f4279417070a8e33829c338abeb42877500377f490abb1495ae6357d50b6a765R92 in the same function doing something like that and implementation looks like this

func RenderUserHeader(ctx *context.Context) {
,
I don't want to break the pattern, if you are suggesting an explicit change in function, that I can.

In short maintainability is the real concern here.

Could you suggest something which is easier for maintainers. I have less knowledge on naming conventions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, the old code is buggy, so I won't say it is a must now.

@@ -1,5 +1,5 @@
<div role="main" aria-label="{{.Title}}" class="page-content repository projects">
<div class="ui container">
<div class="ui{{if .ContextUser.IsOrganization}} container{{end}}">
Copy link
Contributor

Choose a reason for hiding this comment

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

That's too hacky. Why "IsOrganization" means "container"?

Copy link
Member

Choose a reason for hiding this comment

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

If possible, remove the container.

Copy link
Member Author

@puni9869 puni9869 Jun 14, 2023

Choose a reason for hiding this comment

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

Unfortunately container cannot be removed, it is hacky beause we are sharing this between repository and orgransation page. I tried removing it but it will mass the whole project tab design in org view.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, it might be an acceptable compromise then. Didn't check the details.

Copy link
Contributor

Choose a reason for hiding this comment

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

My opinion is that these if-else tricks are too fragile, so I can't vote my approval, while I won't block if others like it and approve.

@puni9869
Copy link
Member Author

puni9869 commented Jul 1, 2023

There are some major conflicts in this PR. Will re-write this in follow up PR.

@puni9869 puni9869 closed this Jul 1, 2023
@puni9869 puni9869 deleted the punit/ISSUE-24871-fixing-user-profile-page-1 branch July 1, 2023 18:25
@GiteaBot GiteaBot removed this from the 1.21.0 milestone Jul 2, 2023
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Sep 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 1 This PR needs approval from one additional maintainer to be merged. size/XL Denotes a PR that changes 500-999 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.

6 participants