8000 [Frontend] Make localstorage read ssh or https correctly by L0veSunshine · Pull Request #11483 · go-gitea/gitea · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[Frontend] Make localstorage read ssh or https correctly #11483

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
May 18, 2020
Merged

[Frontend] Make localstorage read ssh or https correctly #11483

merged 4 commits into from
May 18, 2020

Conversation

L0veSunshine
Copy link
Contributor

The localstorage has stored the user's option of repo's link type, but it don't read correctly.
This pr fix it : when you select the ssh type link, all repo links you enter later are displayed in ssh form.

The ui about this pr is here:
image

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 18, 2020
@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 May 18, 2020
@zeripath zeripath added this to the 1.13.0 milestone May 18, 2020
@L0veSunshine
Update index.js
8a20848 
If not login there is only a "https" button, This commit fix  the "https" button hasn't blue border.
@L0veSunshine
Keep user selected whether or not to log in
695a181
CirnoT
CirnoT reviewed May 18, 2020
View reviewed changes
Copy link
Contributor
@CirnoT CirnoT left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Why have effectively you changed the defaults to ssh by resetting it to that value when user selects HTTPS? Also, there's already code present to save the value on click, I don't see why you would want to set it on initialization.

CirnoT
CirnoT suggested changes May 18, 2020
View reviewed changes
Copy link
Contributor
@CirnoT CirnoT left a comment

Choose a reason for hiding this comment

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

Afraid I have to remove my approval with new changes

@L0veSunshine
Copy link
Contributor Author

Why have effectively you changed the defaults to ssh by resetting it to that value when user selects HTTPS? Also, there's already code present to save the value on click, I don't see why you would want to set it on initialization.

Assume a situation. If you sign in and select "ssh" option. then you log out and view the repo's homepage. Your previous selection will be overwrite to "https".
That is why i do this change. I hope the selection will store without the login status influences.

@CirnoT
Copy link
Contributor
CirnoT commented May 18, 2020 •
edited
Loading
  • Logged out users default to HTTPS and don't see SSH button
  • The change does nothing to fix the issue you mentioned , instead it forces SSH to be default ALWAYS, even if you click HTTPS it will reset to SSH after refresh.

@L0veSunshine
Copy link
Contributor Author
  • It is supposed to do that I believe, logged out users should default to HTTPS
  • The change does nothing to fix that, instead it forces SSH to be default ALWAYS, even if you click HTTPS it will reset to SSH after refresh.

If you click the "https" your default selection will change. The "setitem sentence" will only do when the your selection is "ssh".

@L0veSunshine
Copy link
Contributor Author
L0veSunshine commented May 18, 2020 •
edited
Loading
  • It is supposed to do that I believe, logged out users should default to HTTPS
  • The change does nothing to fix that, instead it forces SSH to be default ALWAYS, even if you click HTTPS it will reset to SSH after refresh.

If you click the "https" your default selection will change. The "setitem sentence" will only do when the your selection is "ssh".

The setitem under this code case 'ssh': . So it will not reset to SSH after refresh. It depends your selection.

@CirnoT
Copy link
Contributor
CirnoT commented May 18, 2020 •
edited
Loading

You are right, it does work.

The check should be moved to $('#repo-clone-https').on('click' most likely however, as I believe there is no guarantee that your setItem will execute after click event has finished; so there is race condition there.

@L0veSunshine

This comment has been minimized.

Sign in to view
@CirnoT
Copy link
Contributor
CirnoT commented May 18, 2020

I meant something like this:

Index: web_src/js/index.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- web_src/js/index.js (revision 695a181b01d50235b80aa6c7077924a0708e5d9c)
+++ web_src/js/index.js (date 1589821225642)
@@ -1109,8 +1109,10 @@
     $('.clone-url').text($(this).data('link'));
     $('#repo-clone-url').val($(this).data('link'));
     $(this).addClass('blue');
-    $('#repo-clone-ssh').removeClass('blue');
-    localStorage.setItem('repo-clone-protocol', 'https');
+    if ($('#repo-clone-ssh').length > 0) {
+      $('#repo-clone-ssh').removeClass('blue');
+      localStorage.setItem('repo-clone-protocol', 'https');
+    }
   });
   $('#repo-clone-url').on('click', function () {
     $(this).select();
@@ -2440,7 +2442,6 @@
           $('#repo-clone-ssh').trigger('click');
         } else {
           $('#repo-clone-https').trigger('click');
-          localStorage.setItem('repo-clone-protocol', 'ssh');
         }
         break;
       default:

@L0veSunshine
Copy link
Contributor Author

I meant something like this:

Index: web_src/js/index.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- web_src/js/index.js (revision 695a181b01d50235b80aa6c7077924a0708e5d9c)
+++ web_src/js/index.js (date 1589821225642)
@@ -1109,8 +1109,10 @@
     $('.clone-url').text($(this).data('link'));
     $('#repo-clone-url').val($(this).data('link'));
     $(this).addClass('blue');
-    $('#repo-clone-ssh').removeClass('blue');
-    localStorage.setItem('repo-clone-protocol', 'https');
+    if ($('#repo-clone-ssh').length > 0) {
+      $('#repo-clone-ssh').removeClass('blue');
+      localStorage.setItem('repo-clone-protocol', 'https');
+    }
   });
   $('#repo-clone-url').on('click', function () {
     $(this).select();
@@ -2440,7 +2442,6 @@
           $('#repo-clone-ssh').trigger('click');
         } else {
           $('#repo-clone-https').trigger('click');
-          localStorage.setItem('repo-clone-protocol', 'ssh');
         }
         break;
       default:

I just thought of this. I will change this.
Thanks for your advice.

@L0veSunshine
Copy link
Contributor Author
L0veSunshine commented May 18, 2020 •
edited
Loading

It is ready for review. And thanks for the advice from @CirnoT.

CirnoT
CirnoT approved these changes May 18, 2020
View reviewed changes
Copy link
Contributor
@CirnoT CirnoT left a comment

Choose a reason for hiding this comment

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

Looks good to me now

6543
6543 approved these changes May 18, 2020
View reviewed changes
Copy link
Member
@6543 6543 left a comment

Choose a reason for hiding this comment

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

let @CirnoT lgtm count

@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 May 18, 2020
@jolheiser jolheiser merged commit eb8abff into go-gitea:master May 18, 2020
@6543
Copy link
Member
6543 commented May 18, 2020

@L0veSunshine pleace send backport :)

@L0veSunshine
Copy link
Contributor Author

Please review the new pr #11489. Also about "index.js" @CirnoT @6543

@L0veSunshine
Copy link
Contributor Author
L0veSunshine commented May 18, 2020 •
edited
Loading

@L0veSunshine pleace send backport :)

I am very sorry, i don't know how to do that.

6543 pushed a commit to 6543-forks/gitea that referenced this pull request May 18, 2020
@L0veSunshine @6543
Make localstorage read ssh or https correctly (go-gitea#11483)
c1f4147 
* Make localstorage read ssh or https correctly

* Update index.js

If not login there is only a "https" button, This commit fix  the "https" button hasn't blue border.

* Keep user selected whether or not to log in

* Update index.js
@6543 6543 mentioned this pull request May 18, 2020
Merged
@6543
Copy link
Member
6543 commented May 18, 2020 •
8000
edited
Loading

@L0veSunshine:

if your local gitea repo hast your fork as remote named origin:

git remote add upstream https://github.com/go-gitea/gitea
git fetch --all
git checkout -f upstream/release/v1.12 -b backport_11483
git cherry-pick eb8abffcc1be777d38a416a030a6e4f42b06ac25
git push -u origin backport_11483

and then make a pull from your fork (branch backport_11483) to the gitea repo branch release/v1.12

witch result in:
#11490

ceers :)

this is of cours only one way to do it ...

@L0veSunshine
Copy link
Contributor Author

@L0veSunshine:

if your local gitea repo hast your fork as remote named origin:

git remote add upstream https://github.com/go-gitea/gitea
git fetch --all
git checkout -f upstream/release/v1.12 -b backport_11483
git cherry-pick eb8abffcc1be777d38a416a030a6e4f42b06ac25
git push -u origin backport_11483

and then make a pull from your fork (branch backport_11483) to the gitea repo branch release/v1.12

witch result in:
#11490

ceers :)

this is of cours only one way to do it ...

Thank you for the tutorial. I will learn. Also thank to your help.

@zeripath zeripath added the backport/done All backports for this PR have been created label May 19, 2020
lafriks pushed a commit that referenced this pull request May 19, 2020
@6543 @L0veSunshine @zeripath
Make localstorage read ssh or https correctly (#11483) (#11490)
1197512 
* Make localstorage read ssh or https correctly

* Update index.js

If not login there is only a "https" button, This commit fix  the "https" button hasn't blue border.

* Keep user selected whether or not to log in

* Update index.js

Co-authored-by: L0veSunshine <xuan199651@gmail.com>
Co-authored-by: zeripath <art27@cantab.net>
ydelafollye pushed a commit to ydelafollye/gitea that referenced this pull request Jul 31, 2020
@L0veSunshine
Make localstorage read ssh or https correctly (go-gitea#11483)
019ce5e 
* Make localstorage read ssh or https correctly

* Update index.js

If not login there is only a "https" button, This commit fix  the "https" button hasn't blue border.

* Keep user selected whether or not to log in

* Update index.js
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@CirnoT CirnoT CirnoT approved these changes

@zeripath zeripath zeripath approved these changes

@6543 6543 6543 approved these changes

Assignees
No one assigned
Labels
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Milestone
1.13.0
Development

Successfully merging this pull request may close these issues.
6 participants
@L0veSunshine @CirnoT @6543 @zeripath @GiteaBot @jolheiser
0