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

Grey-out auto upload options until enabled #2164

Merged
merged 1 commit into from
Apr 10, 2018
Merged

Grey-out auto upload options until enabled #2164

merged 1 commit into from
Apr 10, 2018

Conversation

mario
Copy link
Contributor

@mario mario commented Feb 13, 2018

Signed-off-by: Mario Danic mario@lovelyhq.com

Fixes #1260

Signed-off-by: Mario Danic <mario@lovelyhq.com>
@AndyScherzinger
Copy link
Member

AndyScherzinger commented Feb 13, 2018

👍 looking good code wise

Approved with PullApprove

@mario
Copy link
Contributor Author

mario commented Feb 20, 2018

@tobiasKaminsky :P

@tobiasKaminsky
Copy link
Member

Isn't there a giant discussion how to change this at #2034 ?
Will this fit into your idea @AndyScherzinger?

@AndyScherzinger
Copy link
Member

@tobiasKaminsky this still fits with my idea on the overall change since this is related to the on/off status while my other ideas are related to media/custom type folder pairs. Thus this is still valid 👍

@tobiasKaminsky
Copy link
Member

2018-02-22-145648 2018-02-22-145653

The visual difference between enabled and disabled is too low in my opinion.
It is also confusing that there is an checked, but disabled checkbox (only upload on wifi).
Moreover you can click on save when the switch is disabled (left image), which is also confusing. What is saved?

I still like the idea, but somehow it is now even more confusing for me.

As we have sane defaults for these synced folders, we could enable the folder upon on save and then omit this switch in preferences dialog.

@jancborchardt

@tobiasKaminsky
Copy link
Member

Please feedback by @nextcloud/designers, for me the questions in my last post are still open.

@tobiasKaminsky tobiasKaminsky removed this from the Nextcloud App 3.1.0 milestone Mar 12, 2018
@mario
Copy link
Contributor Author

mario commented Mar 15, 2018

@nextcloud/designers whenever you get a chance :P

@juliusknorr
Copy link
Member

Why not just hide the additional preferences, when the auto upload is disabled and show it when the user enables it. That way it will be clear to the user, that there is nothing to do when it is disabled and the UI would be much less cluttered then.

@jancborchardt
Copy link
Member

Absolutely agree with Julius. Unless the first setting is enabled, everything below is irrelevant and does not need to be shown.

@mario
Copy link
Contributor Author

mario commented Mar 22, 2018

Can we get this merged for 3.1, and then we'll improve for 3.2? @AndyScherzinger @tobiasKaminsky

@AndyScherzinger
Copy link
Member

AndyScherzinger commented Mar 22, 2018

I already approved and would also be fine with merging and improving upon this foundation with 3.2 cc @tobiasKaminsky

@AndyScherzinger
Copy link
Member

@tobiasKaminsky what do you think? Imho this PR could be merged being a better solution than the actual state and improve upon it afterwards?

@tobiasKaminsky
Copy link
Member

As we do not have to rush and next 3.2 release will be in ~6 weeks, we can make it directly better/right, or?

@mario
Copy link
Contributor Author

mario commented Apr 6, 2018

Since this was done on Feb 13, I fail to see why this was not merged...

@jancborchardt jancborchardt self-requested a review April 9, 2018 16:56
Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Let’s do it :) Good first step, better than current state

@AndyScherzinger
Copy link
Member

@tobiasKaminsky ping for review & merge ;-)

@tobiasKaminsky tobiasKaminsky changed the title Fix #1260 Grey-out auto upload options until enabled Apr 10, 2018
@tobiasKaminsky
Copy link
Member

tobiasKaminsky commented Apr 10, 2018

👍
I will open up a following issue to enhance this.

Approved with PullApprove

@tobiasKaminsky tobiasKaminsky merged commit 526c34f into master Apr 10, 2018
@tobiasKaminsky tobiasKaminsky mentioned this pull request Apr 10, 2018
38 tasks
@AndyScherzinger AndyScherzinger deleted the fix-1260 branch April 10, 2018 09:14
@AndyScherzinger
Copy link
Member

@tobiasKaminsky the corresponding issue is assigned to milestone 3.1 -> so should this PR then be backported?

@AndyScherzinger
Copy link
Member

Nevermind, you already wrote that in the roadmap issue for 3.1... sorry...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants