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

When Administrator is logged in it should be visual different from normal user #32894

Closed
wants to merge 2 commits into from

Conversation

xor-gate
Copy link
Contributor

@xor-gate xor-gate commented Dec 18, 2024

This feature enables visual appareance when the logged in user has administrator rights. It colors the toplevel navigation bar red.

See #32885 for discussion

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 18, 2024
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Dec 18, 2024
@github-actions github-actions bot added the modifies/templates This PR modifies the template files label Dec 18, 2024
@xor-gate xor-gate changed the title When user is logged in with Site Administrator access rights a banner should be displayed WIP: When user is logged in with Site Administrator access rights a banner should be displayed Dec 18, 2024
@xor-gate xor-gate changed the title WIP: When user is logged in with Site Administrator access rights a banner should be displayed WIP: When Administrator is logged in it should be visual different from normal user Dec 18, 2024
@xor-gate xor-gate marked this pull request as draft December 18, 2024 20:39
@xor-gate xor-gate changed the title WIP: When Administrator is logged in it should be visual different from normal user When Administrator is logged in it should be visual different from normal user Dec 18, 2024
@lunny
Copy link
Member

lunny commented Dec 18, 2024

Style is not allowed for the change.

@wxiaoguang
Copy link
Contributor

Gitea doesn't use inline-styles.

So either add a new CSS class name, or use tw-xxx and existing color helpers.

@xor-gate
Copy link
Contributor Author

xor-gate commented Dec 19, 2024

Thanks for the feedback, I normally don't do frontend. But I agree with both of you. Need to figure out what CSS classes are available and what needs to be added. I'm running my dev-env on FreeBSD but could not get the frontend to build with node22, i used prebuild frontend from nightly for the draft.

What do you think by putting this behaviour behind a config option, or making it default? Then I also need some backend work to be investigated and implemented. Some people will not going to like this when it is not configurable. But its a matter of taste.

@wxiaoguang
Copy link
Contributor

Before making more changes, I think we need to figure how to make the UI overall look good.

To be honest, such color pattern doesn't look to me .... somewhat ugly.

Could you share how other softwares do? What's their color palette?

And maybe @silverwind could have some ideas.

image

@silverwind
Copy link
Member

silverwind commented Dec 19, 2024

I don't think this is needed and just coloring red is far too crude. Admin can see whether they are admin by opening the user menu where "Site Administration" will be present.

Only other idea is to overlay a cog icon over the user avatar on top right.

@wxiaoguang
Copy link
Contributor

Admin can see whether they are admin by opening the user menu where "Site Administration" will be present.

It doesn't resolve user's problem.

The real requirement is to make the "admin UI" more eye-catching to make sure admin users could know they are using an admin account, to avoid making some over-permission operations in unrelated repositories.

Only other idea is to overlay a cog icon over the user avatar on top right.

It's better to have some more eye-catching UI differences.

@silverwind
Copy link
Member

It's better to have some more eye-catching UI differences.

I disagree. It seems over the top and unprofessional to do things like color the navbar.

@xor-gate
Copy link
Contributor Author

It's better to have some more eye-catching UI differences.

I disagree. It seems over the top and unprofessional to do things like color the navbar.

It was just a draft after comment from @lunny in the ticket. I would prefer a small bar above the navbar with color and text "Logged in as Site Administrator". After discussing and showing this, the navbar color is also ugly compared to the other UI elements. I'm no UI or frontend expert, but it should feel and look nice.

@xor-gate
Copy link
Contributor Author

Something like this (AI generated):

image

Could you share how other softwares do? What's their color palette?

Unfortunatly I have no example from other software (yet)

@silverwind
Copy link
Member

silverwind commented Dec 19, 2024

The only thing I can accept is a small icon overlay over the avatar. Cog is a common iconology to display admin status: https://www.google.com/search?q=administrator+icon. E.g. like this:

      -----
------| c | 
|     -----
|   u   |
|       |
---------

The relevant CSS can be copied from the notification icon's blue dot.

@xor-gate
Copy link
Contributor Author

Okay so there is no general consensus. Then we leave it as-is.

@xor-gate xor-gate closed this Dec 19, 2024
@xor-gate xor-gate deleted the issue-32885 branch December 19, 2024 11:27
@lunny
Copy link
Member

lunny commented Dec 19, 2024

I don't think this is needed and just coloring red is far too crude. Admin can see whether they are admin by opening the user menu where "Site Administration" will be present.

Only other idea is to overlay a cog icon over the user avatar on top right.

This is the same as my second idea in the original issue.

@wxiaoguang
Copy link
Contributor

-> Improve navbar: add "admin" tip, add "active" style #32927

@xor-gate
Copy link
Contributor Author

xor-gate commented Dec 20, 2024

@wxiaoguang you are an UI expert, this is much better then coloring the navbar. That felt not very natural, but was to discus the idea. Thanks for considering this feature request! Up to v1.24.0 🚀 and beyond the stars.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. modifies/templates This PR modifies the template files size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants