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 disabled open in vscode menu when disabling download source from UI #20713

Merged
merged 3 commits into from
Aug 12, 2022

Conversation

lunny
Copy link
Member

@lunny lunny commented Aug 8, 2022

@lunny lunny added type/bug skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. labels Aug 8, 2022
@lunny lunny requested review from Gusted and silverwind August 8, 2022 15:54
@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (main@ccf03e1). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 9815cf9 differs from pull request most recent head ee2004f. Consider uploading reports for the commit ee2004f to get more accurate results

@@           Coverage Diff           @@
##             main   #20713   +/-   ##
=======================================
  Coverage        ?   46.92%           
=======================================
  Files           ?      981           
  Lines           ?   136262           
  Branches        ?        0           
=======================================
  Hits            ?    63937           
  Misses          ?    64477           
  Partials        ?     7848           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 8, 2022
@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 Aug 8, 2022
</div>
</button>
{{else}}
<button id="download-btn" onclick="window.location='vscode://vscode.git/clone?url={{$.RepoCloneLink.HTTPS}}'" class="ui basic small compact jump icon button tooltip" data-content="{{.locale.Tr "repo.clone_in_vsc"}}" data-position="top right">
Copy link
Contributor

Choose a reason for hiding this comment

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

Inline scripts, is that necessary? Is it possible to wrap a <a href=....> into this element?

Copy link
Member Author

Choose a reason for hiding this comment

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

I cannot compact all the buttons when using a tag. I have found other place to use onclick, so just follow that.

Copy link
Member

@lafriks lafriks left a comment

Choose a reason for hiding this comment

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

DisableDownloadSourceArchives check is now missing and there is duplicate vscode button & menu

@lunny
Copy link
Member Author

lunny commented Aug 8, 2022

DisableDownloadSourceArchives check is now missing and there is duplicate vscode button & menu

fixed.

templates/repo/home.tmpl Outdated Show resolved Hide resolved
templates/repo/home.tmpl Outdated Show resolved Hide resolved
@silverwind
Copy link
Member

silverwind commented Aug 8, 2022

I think we should keep this a menu like #19999 did and not add more seldomly used buttons like this vscode button to the always-rendered view. E.g. revert this change, rename button to "More Actions" with new icon and then just add the hide conditions on the individual items that trigger downloads, unconditionally rendering the vscode menu item.

@lafriks
Copy link
Member

lafriks commented Aug 8, 2022

But there is no point in menu if there is single item in it

@silverwind
Copy link
Member

But there is no point in menu if there is single item in it

I'd argue there still is, for sake of consistent user experience where a menu doesn't magically transform into a single-action button.

@lunny
Copy link
Member Author

lunny commented Aug 9, 2022

But there is no point in menu if there is single item in it

I'd argue there still is, for sake of consistent user experience where a menu doesn't magically transform into a single-action button.

I think it's just a habbit problem. The most next action to click the copy button is to paste it in a client commandline or software and then clone the repository. But now, if you are using vscode(about 80% developers used), clicking the second button will finish them more frequently. In future, VSCode button could also become another menus to support gitpod or other offline/online editors which is different from the download archives behaviors.

@silverwind
Copy link
Member

I personally do not use VSCode, so this feels like unnecessary preferential treatment of one editor over others. After #19999, will have citation actions as well in that menu and I'm sure there will be more stuff to come. So I still prefer that button to be tucked into that menu.

@lunny
Copy link
Member Author

lunny commented Aug 9, 2022

I personally do not use VSCode, so this feels like unnecessary preferential treatment of one editor over others. After #19999, will have citation actions as well in that menu and I'm sure there will be more stuff to come. So I still prefer that button to be tucked into that menu.

That's my first second version to hide the three menus and left only one menu in the dropdown.

@silverwind
Copy link
Member

That's my first version to hide the three menus and left only one menu in the dropdown.

That's what I'd approve, e.g. just move the {{if not $.DisableDownloadSourceArchives}} condition inside the menu so it affects the relevant items only.

@lafriks
Copy link
Member

lafriks commented Aug 9, 2022

If there is planned adding more menu options than yes, it's no point in making it currently as button and then making it back in menu when other actions are added

@Gusted
Copy link
Contributor

Gusted commented Aug 9, 2022

So, why is the VSCode item a new button when the menu is there?
image

In the same boat as silverwind, I'm not using VSCode and just prefer to have it tucked away in the menu. More items are currently planned to come into that menu, so don't see a good reason to move away from that menu.

@lafriks
Copy link
Member

lafriks commented Aug 9, 2022

@Gusted it's just one of experimental proposals, if you look over this PR you will see multiple iterations here :)

@lafriks
Copy link
Member

lafriks commented Aug 9, 2022

But as more items are soon to be added here I agree we should move back to inital commit 9815cf9

@lunny
Copy link
Member Author

lunny commented Aug 11, 2022

I rollbacked to my first version since nobody against that for two days. @lafriks @silverwind @Gusted

@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 Aug 11, 2022
@lunny
Copy link
Member Author

lunny commented Aug 12, 2022

make L-G-T-M work

@lunny lunny merged commit d26b652 into go-gitea:main Aug 12, 2022
@lunny lunny deleted the lunny/fix_dl_btn branch August 12, 2022 05:16
zjjhot added a commit to zjjhot/gitea that referenced this pull request Aug 12, 2022
* giteaofficial/main:
  Fix disabled open in vscode menu when disabling download source from UI (go-gitea#20713)
vsysoev pushed a commit to IntegraSDL/gitea that referenced this pull request Aug 28, 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.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants