Skip to content

resolve issue with sort icons on admin/users and admin/runners #24360

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

Merged
merged 8 commits into from
Sep 12, 2023

Conversation

jladbrook
Copy link
Contributor

@jladbrook jladbrook commented Apr 26, 2023

Fixes #24327 to avoid the sort icon changing the table header over multiple lines and adds missing sort icons on the runners page.

Fixed User Admin Page

image

Updated Runners Page

image

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 26, 2023
@pull-request-size pull-request-size bot added size/S and removed size/M labels Apr 26, 2023
@lunny lunny added the type/enhancement An improvement of existing functionality label Apr 27, 2023
@silverwind
Copy link
Member

Not directly related or requested, but I feel the icon should be vertically centered with the text:

image

@silverwind
Copy link
Member

silverwind commented Apr 28, 2023

It seems #24370 had adressed some of the same issues as this PR. @wxiaoguang which parts of this PR do we still need?

@silverwind
Copy link
Member

Looking at this admin user table, I can't help but feel that there are too many columns to fit in the available space, so either remove some columns or increase page width, imho. Maybe the checkbox-only column titles could be reduced to svg icons only to save space.

@wxiaoguang
Copy link
Contributor

which parts of this PR do we still need?

If I understand correctly, this PR is mainly for the "sort" icons/logic?

@silverwind
Copy link
Member

I get the same impression, so the CSS changes should potentially be backed out now.

@denyskon
Copy link
Member

denyskon commented Sep 10, 2023

@jladbrook Looks like the first issue is resolved now (193dce5). Could you refresh your PR and only keep the fix for the runners page?

@denyskon denyskon added type/bug issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail labels Sep 10, 2023
@jladbrook
Copy link
Contributor Author

@denyskon that's done

@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 Sep 11, 2023
@denyskon denyskon added backport/v1.20 This PR should be backported to Gitea 1.20 and removed type/enhancement An improvement of existing functionality issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail labels Sep 11, 2023
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Sep 12, 2023

Has the alignment problem #24360 (comment) been resolved?

If yes, please update the screenshot.

If no and need help, feel free to ask ~~


Update: IMO it doesn't need to be considered as "bug" or need to be backported.

@wxiaoguang wxiaoguang added type/enhancement An improvement of existing functionality and removed type/bug backport/v1.20 This PR should be backported to Gitea 1.20 labels Sep 12, 2023
@wxiaoguang wxiaoguang added the topic/ui Change the appearance of the Gitea UI label Sep 12, 2023
@github-actions github-actions bot removed the topic/ui Change the appearance of the Gitea UI label Sep 12, 2023
@github-actions github-actions bot added the topic/ui Change the appearance of the Gitea UI label Sep 12, 2023
@jladbrook
Copy link
Contributor Author

Has the alignment problem #24360 (comment) been resolved?

If yes, please update the screenshot.

If no and need help, feel free to ask ~~

Update: IMO it doesn't need to be considered as "bug" or need to be backported.

Hi @wxiaoguang that wasn't included in the PR previously. I've had a quick look and updated to include a change to achieve:

image

This was a minor change to base.css - if there is a better way to achieve please let me know and I'll update accordingly.

Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

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

Thank you! It looks good to me now (and I will take a test later 😄 )


Update: the new margin affects other UI. I will propose a fix soon.

@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 Sep 12, 2023
@jladbrook
Copy link
Contributor Author

Update: the new margin affects other UI. I will propose a fix soon.

Oh sorry I did check around in my changed version and didn't notice the change - what page(s) was affected?

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Sep 12, 2023

That's really strange. According to my test, without margin-top: 0.25rem; , the old UI just looks fine.

But when the margin-top: 0.25rem; is added, it makes the UI misalign.

I don't know why your first screenshot shows that the icon misalignment. Maybe it's better to remove the margin-top: 0.25rem; and leave the problem to the future.


The old UI on my side

image

The new UI with margin-top: 0.25rem;

image

@github-actions github-actions bot removed the topic/ui Change the appearance of the Gitea UI label Sep 12, 2023
@wxiaoguang wxiaoguang enabled auto-merge (squash) September 12, 2023 11:58
@wxiaoguang wxiaoguang added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Sep 12, 2023
@jladbrook
Copy link
Contributor Author

Strange - I'm using a fairly recent version of Chrome, although it shows the same in Brave and FireFox. I assume it's something to do with my install. I've reverted the change.

Thanks for the feedback.

@wxiaoguang
Copy link
Contributor

Strange - I'm using a fairly recent version of Chrome, although it shows the same in Brave and FireFox. I assume it's something to do with my install.

Maybe it's related to your OS & font.

Indeed, the legacy "margin / vertical-align" layout is not ideal and has various problems. Ideally the layout should be refactored to a new approach later -- it will be another story.

@wxiaoguang wxiaoguang merged commit e33f112 into go-gitea:main Sep 12, 2023
@GiteaBot GiteaBot added this to the 1.22.0 milestone Sep 12, 2023
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Sep 12, 2023
@denyskon
Copy link
Member

@wxiaoguang Why are you against a backport? This fix adds the missing sort icon to the runners table and improves sorting. I think this isn't anything that has the potential to break something

@wxiaoguang
Copy link
Contributor

IMO each backport comes with risk: even if there is no conflict, the backported code might not work, and no one would really test the backport.

So, if a fixed problem is not very important (eg: no matter with or without an "icon", the "list" still works), I would prefer to not backport.

But that's just my opinion, free free to backport if you think it is important.

zjjhot added a commit to zjjhot/gitea that referenced this pull request Sep 13, 2023
* giteaofficial/main:
  fix media description render for orgmode (go-gitea#26895)
  Show OpenID Connect and OAuth on signup page (go-gitea#20242)
  Update status and code index after changing the default branch (go-gitea#27018)
  add sparse url in cargo package guide (go-gitea#26937)
  Add missing 404 response to Swagger (go-gitea#27038)
  Fix object storage path handling (go-gitea#27024)
  [skip ci] Updated translations via Crowdin
  bump rootful to 16gb
  setup go in the nightly release pipeline
  Speed up nightly builds (go-gitea#27045)
  Improve repo/user/org search  (go-gitea#27030)
  Use Actuated.dev runner for nightly builds
  update snap package (go-gitea#27021)
  resolve issue with sort icons on admin/users and admin/runners (go-gitea#24360)
@delvh delvh modified the milestones: 1.22.0, 1.21.0 Sep 13, 2023
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Dec 11, 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. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The sort icon problem in admin setting pages
7 participants