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

Ensure isSSH is set whenever DISABLE_HTTP_GIT is set #19022

Closed

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented Mar 7, 2022

In clone_buttons.tmpl the defer block sets the javascript isSSH variable.
When DISABLE_HTTP_GIT is set we should always set this to true.

Fix #19020

Signed-off-by: Andrew Thornton [email protected]

In clone_buttons.tmpl the defer block sets the javascript isSSH variable.
When DISABLE_HTTP_GIT is set we should always set this to true.

Fix go-gitea#10920

Signed-off-by: Andrew Thornton <[email protected]>
@zeripath
Copy link
Contributor Author

zeripath commented Mar 7, 2022

Great lint is not allowing the obvious solution.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 7, 2022
Signed-off-by: Andrew Thornton <[email protected]>
{{if not (and $.DisableHTTP $.DisableSSH)}}
<script defer>
const isSSH = localStorage.getItem('repo-clone-protocol') === 'ssh';
const isSSH = httpsDisabled || localStorage.getItem('repo-clone-protocol') === 'ssh';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const isSSH = httpsDisabled || localStorage.getItem('repo-clone-protocol') === 'ssh';
const isSSH = {{if $.DisableHTTP}}true || {{end}}localStorage.getItem('repo-clone-protocol') === 'ssh';

A suggestion: this changes this PR to one line vs 10. Feel free to take it or discard, although if discarded then the above code chunk could have some de-duplication.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that will fail our lint

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 tried this orignally)

Copy link
Member

Choose a reason for hiding this comment

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

But how about when they disabled both HTTP and SSH? Just use the UI?

Copy link
Member

Choose a reason for hiding this comment

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

😭 ah, I see.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can bypass the JS linter for some code blocks. See the head.tmpl

image

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Mar 8, 2022

I think these JS code should be refactored. For example:

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/script

Warning: This attribute  (defer) must not be used if the src attribute is absent (i.e. for inline scripts), in this case it would have no effect.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Mar 8, 2022

It could be refactored to the following code.

Benefits:

  1. No more global variable pollution, every variable is in their own scope pageData and (()=>{})()
  2. The major code is still linted by linter.
  3. DisableHTTP can be encoded by html template to JSON automatically.
{{if not (and $.DisableHTTP $.DisableSSH)}}
	<script>
		<!-- /* eslint-disable */ -->
		window.config.pageData['repoCloneButtons'] = {httpsDisabled: {{$.DisableHTTP}}};
	</script>
	<script>
		(() => {
			const tmplData = window.config.pageData.repoCloneButtons;
			const isSSH = tmplData.httpsDisabled || localStorage.getItem('repo-clone-protocol') === 'ssh';
			const sshButton = document.getElementById('repo-clone-ssh');
			const httpsButton = document.getElementById('repo-clone-https');
			const input = document.getElementById('repo-clone-url');
			if (input) input.value = (isSSH ? sshButton : httpsButton).getAttribute('data-link');
			if (sshButton) sshButton.classList[isSSH ? 'add' : 'remove']('primary');
			if (httpsButton) httpsButton.classList[isSSH ? 'remove' : 'add']('primary');
			setTimeout(() => {
				if (sshButton) sshButton.classList.remove('no-transition');
				if (httpsButton) httpsButton.classList.remove('no-transition');
			}, 100);
		})();
	</script>
{{end}}

@zeripath
Copy link
Contributor Author

zeripath commented Mar 8, 2022

I shall defer to @wxiaoguang

@zeripath zeripath closed this Mar 8, 2022
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Mar 8, 2022

The new #19028 is based on this one 🙏, it could pass the lint.

@zeripath zeripath deleted the fix-10920-show-ssh-on-quickview branch March 8, 2022 09:55
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
@wxiaoguang wxiaoguang removed this from the 1.17.0 milestone Jun 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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.

"Quick Guide" commands always access repo over http(s)
5 participants