-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Files external settings polish #787
Conversation
icewind1991
commented
Aug 9, 2016
- Hide "global credentails" and dependency notes when a user can't create mounts
- hide "External Storage" from personal settings if there is nothing to show
@icewind1991, thanks for your PR! By analyzing the annotation information on this pull request, we identified @jancborchardt, @Xenopathic and @PVince81 to be potential reviewers |
var mainForm = $('#files_external'); | ||
if (result.length === 0 && mainForm.attr('data-can-create') === 'false') { | ||
mainForm.hide(); | ||
$('a[href="#external-storage"]').parent().hide(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works but is a little bit ugly because the time that loading takes is around 0.5 seconds here and then the navigation bar on the left magically removes one item.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not much we can easily do about it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a idea by @nickvergessen, shouldn't it be possible to already decide on PHP level if we want to show the settings or not and don't return a settings page here https://github.com/nextcloud/server/blob/master/apps/files_external/personal.php if we don't want to show the settings?
Works but https://github.com/nextcloud/server/pull/787/files#r74160076 makes it a little bit of a bad UX. |
a488b94
to
1522beb
Compare
I fixed some casing stuff too. This addresses some issues of #735 |
Agree... So what should we do here? How to move forward? @nextcloud/designers opinions? |
Should we split it up and first merge the uncontroversial part like fix whitespace, non italic text, "using OC login", no note,... And put the "hide if empty" part in a new PR for further discussion so that we have at least something for the RC? |
1522beb
to
8f751c7
Compare
It now hides the "Files External" section on the server side |
Works great, @icewind1991 ! Just one small thing I discovered while testing. The drop-down to select the external storage has a option "ownCloud". I would suggest to change it to "Nextcloud / ownCloud". |
Changes it to "Nextcloud", showing both only adds clutter imo |
👍 |
dcdc6b3
to
9a7193c
Compare
I rebased because of the conflict (was only a string change in a file that was moved for the admin page split PR - #796) I tested it and it works 👍 |
👍 |
|
please backport :-) |
On it 💪 |
Backport is in #902 👊 |
We lost at least two commits, probably during a rebase.
@icewind1991 can you have a look and bring this commits back? Maybe you have a local copy? |
…gs-polish Files external settings polish