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

only show "Add to your Nextcloud" on share link if federation is acti… #13261

Merged
merged 4 commits into from
Feb 1, 2019

Conversation

violoncelloCH
Copy link
Member

@violoncelloCH violoncelloCH commented Dec 25, 2018

…vated

This only hides "Add to your Nextcloud" if the checkmark for federated sharing in settings is unchecked. Deactivating the federation app does not hide these settings and does not hide the "Add to your Nextcloud"-option as well.

fixes #12395

apps/files_sharing/lib/Controller/ShareController.php Outdated Show resolved Hide resolved
@violoncelloCH
Copy link
Member Author

@juliushaertl like this? ^

@violoncelloCH
Copy link
Member Author

@juliushaertl @ChristophWurst @skjnldsv can we have another review here?

Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

Much cleaner :)

@violoncelloCH
Copy link
Member Author

may I add myself as author to the file header now? or what do you consider a "substantial change"?

@skjnldsv
Copy link
Member

may I add myself as author to the file header now? or what do you consider a "substantial change"?

You can always add yourself to the header. I'd even say you always should. But most of the time people forget :/

@violoncelloCH
Copy link
Member Author

You can always add yourself to the header. I'd even say you always should. But most of the time people forget :/

good to know! I've done it now...

@kesselb
Copy link
Contributor

kesselb commented Jan 30, 2019

$responseComposer = [];

if ($shareIsFolder) {
	$responseComposer[] = new SimpleMenuAction('download', $this->l10n->t('Download all files'), 'icon-download-white', $shareTmpl['downloadURL'], 0);
	$responseComposer[] = new SimpleMenuAction('download', $this->l10n->t('Download all files'), 'icon-download', $shareTmpl['downloadURL'], 10, $shareTmpl['fileSize']);
} else {
	$responseComposer[] = new SimpleMenuAction('download', $this->l10n->t('Download'), 'icon-download-white', $shareTmpl['downloadURL'], 0);
	$responseComposer[] = new SimpleMenuAction('download', $this->l10n->t('Download'), 'icon-download', $shareTmpl['downloadURL'], 10, $shareTmpl['fileSize']);
}

$responseComposer[] = new LinkMenuAction($this->l10n->t('Direct link'), 'icon-public', $shareTmpl['previewURL']);
if ($this->federatedShareProvider->isOutgoingServer2serverShareEnabled()) {
	$responseComposer[] = new ExternalShareMenuAction($this->l10n->t('Add to your Nextcloud'), 'icon-external', $shareTmpl['owner'], $shareTmpl['displayName'], $shareTmpl['filename']);
}

I think it looks cleaner if you do it like above. Yes the download buttons looks similar but they are different. If you dont agree just ignore 🤣

Copy link
Contributor

@kesselb kesselb left a comment

Choose a reason for hiding this comment

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

Well done 👍

@violoncelloCH violoncelloCH dismissed juliusknorr’s stale review January 30, 2019 16:05

outdated / requested changes implemented

@ChristophWurst ChristophWurst added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jan 30, 2019
@violoncelloCH
Copy link
Member Author

@danielkesselberg thank you! I get your point; the variables are only assigned and use once, so not really necessary -- however I think it's easier to read the code with the variables... But if you want the code to be more compact I can change it :)

@kesselb
Copy link
Contributor

kesselb commented Jan 30, 2019

@danielkesselberg thank you! I get your point; the variables are only assigned and use once, so not really necessary -- however I think it's easier to read the code with the variables... But if you want the code to be more compact I can change it :)

Up to you ;-) If someone approves a pull request and suggests changes they are not mandatory (i think they are not mandatory but could be wrong 🤣)

@violoncelloCH
Copy link
Member Author

what's wrong with drone here?

@@ -7,6 +7,7 @@
* @author Björn Schießle <bjoern@schiessle.org>
* @author Georg Ehrke <oc.list@georgehrke.com>
* @author Joas Schilling <coding@schilljs.com>
* @author Jonas Sulzer <jonas@violoncello.ch>
Copy link
Member

Choose a reason for hiding this comment

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

🎉

@skjnldsv
Copy link
Member

@violoncelloCH can you rebase? Drone will recreate itself :)

…vated

Signed-off-by: Jonas Sulzer <jonas@violoncello.ch>
Signed-off-by: Jonas Sulzer <jonas@violoncello.ch>
Signed-off-by: Jonas Sulzer <jonas@violoncello.ch>
Signed-off-by: Jonas Sulzer <jonas@violoncello.ch>
@violoncelloCH
Copy link
Member Author

rebase done...

@violoncelloCH
Copy link
Member Author

looks like there is always one randomly failing drone test... :/

@MorrisJobke
Copy link
Member

They are unrelated - it was a codecov failure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish feature: federation feature: sharing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disable "Add to your nextcloud" option in public share links
6 participants