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

Disable unnecessary SSH elements #18482

Closed
wants to merge 2 commits into from

Conversation

pboguslawski
Copy link
Contributor

This mod fixes disabling unnecessary SSH elements.

Related: #13129
Author-Change-Id: IB#1115249

This mod fixes disabling unnecessary SSH elements.

Related: go-gitea#13129
Author-Change-Id: IB#1115249
@@ -517,7 +517,7 @@ appearance = Appearance
password = Password
security = Security
avatar = Avatar
ssh_gpg_keys = SSH / GPG Keys
keys = Keys
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO: I think Keys is too vague to be a navbar item.

Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect that we might need to have a GPG Keys menu option if SSH is absolutely disabled.

Copy link
Contributor Author

@pboguslawski pboguslawski Feb 1, 2022

Choose a reason for hiding this comment

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

Menu item should not contain technology name (SSH, GPG, etc.) because disabled items should be removed from page (clean UI should not contain stuff user don't use). It should also be short and clear. "Keys" are fine IHMO and not more vague than other existing items there. Propose better name if you can't accept simple "Keys".

Copy link
Contributor

Choose a reason for hiding this comment

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

If you check GitHub's UI and Gitlab's UI you'll find that the technology name is included in the tab name.

We have to include the technology type to disambiguate the type of keys.

There are authentication keys - eg. Yubikeys which are not configured on this tab. If you call it Keys then this will become confusing.

Copy link
Contributor Author

@pboguslawski pboguslawski Feb 1, 2022

Choose a reason for hiding this comment

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

If you check GitHub's UI and Gitlab's UI you'll find that the technology name is included in the tab name.

Gitea may have cleaner UI :)

Propose better name if you can't accept simple "Keys" (avoid leaving unnecessary items like SSH words when SSH is disabled in config).

There are authentication keys - eg. Yubikeys which are not configured on this tab.

SSH keys are also authentication keys. Consider merging all this auth/sign/token stuff in one page like "Security" in the future. Or maybe move all this stuff to existing Account page if space allows.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry didn't noticed GPG Keys proposal - will extend mod with this. Still consider merging all this auth/sign/token stuff in one page in user settings in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

<div class="ui blue tiny show-panel button" data-panel="#add-ssh-principal-panel">{{.i18n.Tr "settings.add_new_principal"}}</div>
{{else}}
<div class="ui blue tiny button disabled">{{.i18n.Tr "settings.ssh_disabled"}}</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think for the user's information these should be kept.

Copy link
Contributor Author

@pboguslawski pboguslawski Feb 1, 2022

Choose a reason for hiding this comment

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

There should be no unnecessary items in UI present to make it as clean as possible. If owner disables SSH in the application there is no point in leaving SSH items in UI.

Copy link
Contributor

Choose a reason for hiding this comment

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

From how I see it, if I happens to be on some gitea instance and I didn't saw such menu(and didn't had the knowledge it could be disabled) I would open a bug report that this menu is not present, I think it's safe to inform the users that it isn't enabled on the instance.

Copy link
Contributor

Choose a reason for hiding this comment

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

SSH is de-facto standard for Git operations nowadays and is expected to be supported universally everywhere. User should most definitely be informed that SSH is not available.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would open a bug report that this menu is not present,

Reading system manual should be enough.

There should be no unnecessary items in UI present to make it as clean as possible. We prefer to give users clean UI without unnecessary information noise and will hide all SSH stuff. Feel free to close this PR if you don't like it.

and is expected to be supported universally everywhere

We don't expect using git over SSH so the above is false :).

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't expect using git over SSH so the above is false :).

Your singular needs do not represent the bigger picture. Please understand that Gitea is a project aimed at far bigger audience - for a feature to be added it should take into consideration that fact. I see no bigger issues with this PR other than the mentioned notification about disabled SSH being removed.

Copy link
Contributor Author

@pboguslawski pboguslawski Feb 2, 2022

Choose a reason for hiding this comment

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

Your singular needs[...]

We only push PRs that may be valuable for bigger audience. Clean UI is IHMO. As mentioned above - fell free to close this PR if you don't like it.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 31, 2022
Copy link
Contributor

@zeripath zeripath left a comment

Choose a reason for hiding this comment

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

We're gonna need to reindent these files I think

@@ -517,7 +517,7 @@ appearance = Appearance
password = Password
security = Security
avatar = Avatar
ssh_gpg_keys = SSH / GPG Keys
keys = Keys
Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect that we might need to have a GPG Keys menu option if SSH is absolutely disabled.

@pboguslawski
Copy link
Contributor Author

pboguslawski commented Feb 1, 2022

I suspect that we might need to have a GPG Keys menu option if SSH is absolutely disabled.

If SSH is disabled, menu GPG Keys with GPG keys is still present after this mod is applied. If SSH is enabled, both SSH and GPG keys are present under SSH / GPG Keys menu.

Fixes: a82f4ee
Related: go-gitea#18482
Author-Change-Id: IB#1115249
@techknowlogick techknowlogick added this to the 1.17.0 milestone Feb 2, 2022
@stale
Copy link

stale bot commented Apr 18, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions.

@stale stale bot added the issue/stale label Apr 18, 2022
@lunny lunny added the issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented label Apr 19, 2022
@stale stale bot removed the issue/stale label Apr 19, 2022
@techknowlogick techknowlogick modified the milestones: 1.17.0, 1.18.0 Jun 8, 2022
@lunny lunny modified the milestones: 1.18.0, 1.19.0 Oct 17, 2022
@lunny lunny modified the milestones: 1.19.0, 1.20.0 Feb 3, 2023
@wxiaoguang
Copy link
Contributor

It has been stale for a long time, I think it could be closed.

ps: I also agree that usually it should avoid "hiding" UI details, which makes the UI fragile and difficult to test.

A whole page could be hidden totally if a feature is disabled, but hiding UI elements again and again seems whack-a-mole game.

@wxiaoguang wxiaoguang closed this May 10, 2023
@GiteaBot GiteaBot removed this from the 1.20.0 milestone May 10, 2023
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Aug 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants