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.

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

Transfer ownership function polishing #18270

Merged
merged 8 commits into from
Dec 17, 2019
Merged

Transfer ownership function polishing #18270

merged 8 commits into from
Dec 17, 2019

Conversation

wiswedel
Copy link
Contributor

@wiswedel wiswedel commented Dec 6, 2019

Signed-off-by: Sascha Wiswedel sascha.wiswedel@nextcloud.com

@ChristophWurst ChristophWurst added this to the Nextcloud 18 milestone Dec 6, 2019
@wiswedel
Copy link
Contributor Author

wiswedel commented Dec 6, 2019

The padding after the numbers is quite narrow, but I'm incapable of fixing that. Can someone please have a look at that?

image

I think it's logically safe to push the solution into this polishing PR.

@ChristophWurst
Copy link
Member

The padding after the numbers is quite narrow, but I'm incapable of fixing that. Can someone please have a look at that?

This is something that should be changed/fixed broadly with our design rules and not just here so we can have a consistent appearance.

@wiswedel
Copy link
Contributor Author

wiswedel commented Dec 9, 2019

The padding after the numbers is quite narrow, but I'm incapable of fixing that. Can someone please have a look at that?

This is something that should be changed/fixed broadly with our design rules and not just here so we can have a consistent appearance.

@ChristophWurst Fine for me. How do I get this onto that design rules fix backlog?

@ChristophWurst
Copy link
Member

The padding after the numbers is quite narrow, but I'm incapable of fixing that. Can someone please have a look at that?

This is something that should be changed/fixed broadly with our design rules and not just here so we can have a consistent appearance.

cc @nextcloud/designers

@jancborchardt
Copy link
Member

The list styling is a strange way to do this here, so that’s why it looks strange. Nothing wrong with the list styling, instead the flow should be adjusted. Mostly this is about wording:

Folder to transfer
( Select )

New owner
[ Search users ]

( Transfer foldername to username)

Transfer button on the bottom should be greyed out until both folder and recipient are selected.

@wiswedel
Copy link
Contributor Author

( Transfer foldername to username)

I'm not capable of implementing this. Anyone else?

This was referenced Dec 11, 2019
Merged
Merged
@wiswedel wiswedel marked this pull request as ready for review December 12, 2019 15:23
@wiswedel wiswedel added 18-feedback 3. to review Waiting for reviews and removed 2. developing Work in progress labels Dec 12, 2019
@gary-kim
Copy link
Member

gary-kim commented Dec 15, 2019 •
edited
Loading

@wiswedel How is this?
Screenshot from 2019-12-15 17-29-00
P.S. I can rebase this when I have a moment.

@rullzer
Copy link
Member

rullzer commented Dec 16, 2019

@gary-kim awesome. Lets go for that

@gary-kim gary-kim force-pushed the wiswedel/transferOwnership/polishing branch from 3e60c83 to 6df800e Compare December 16, 2019 08:25
@gary-kim
Copy link
Member

Made a few changes with @jancborchardt
@ChristophWurst Do you want to take another look?
Screenshot from 2019-12-16 16-28-21

ChristophWurst
ChristophWurst requested changes Dec 16, 2019
View reviewed changes
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

The code changes look good

Touched files like apps/settings/js/vue-3.js seem unrelated? Did you install npm dependencies before the build?

@ChristophWurst
Copy link
Member

Also, this needs a rebase to resolve the conflicts

@gary-kim gary-kim force-pushed the wiswedel/transferOwnership/polishing branch from 6df800e to 771fce2 Compare December 16, 2019 09:14
@gary-kim
Copy link
Member

Rebased.

Touched files like apps/settings/js/vue-3.js seem unrelated? Did you install npm dependencies before the build?

The CI webpack-build succeeded so it looks like those files should be touched? Let's see if the new build passes.

@ChristophWurst
Copy link
Member

Yes, let's see. +95 −10,994 is very suspicious, though 🤔

@gary-kim gary-kim force-pushed the wiswedel/transferOwnership/polishing branch 2 times, most recently from c681fcf to 3f4d24e Compare December 16, 2019 13:03
@gary-kim
Copy link
Member

@ChristophWurst Looks like the webpack-build is still passing. I'm also not entirely sure what part of the change resulted in all those removals.

jancborchardt
jancborchardt approved these changes Dec 17, 2019
View reviewed changes
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.

Very nice and simple design-wise now. :) Thanks a lot @gary-kim and @wiswedel!

@gary-kim gary-kim mentioned this pull request Dec 17, 2019
Merged
wiswedel and others added 8 commits December 17, 2019 10:38
@wiswedel @rullzer
Transfer ownership function polishing
f1319e1 
Signed-off-by: Sascha Wiswedel <sascha.wiswedel@nextcloud.com>
@wiswedel @rullzer
notification polishing
f567566 
Signed-off-by: Sascha Wiswedel <sascha.wiswedel@nextcloud.com>
@wiswedel @rullzer
transfer ownership dialog word choice
a64b5de 
Use Reject instead of Decline


Signed-off-by: Sascha Wiswedel <sascha.wiswedel@nextcloud.com>
@wiswedel @rullzer
added patience note for sharee
6a6b6dd 
note about incoming transfer taken out by background job

resolves #18272

Signed-off-by: Sascha Wiswedel <sascha.wiswedel@nextcloud.com>
@wiswedel @rullzer
rephrase "Target user" to "new owner"
bdceb0d 
Signed-off-by: Sascha Wiswedel <sascha.wiswedel@nextcloud.com>
@gary-kim @rullzer
Show path and uid on transfer button
09f1c63 
Signed-off-by: Gary Kim <gary@garykim.dev>
@gary-kim @rullzer
Clean up publishing
e1382e2 
Signed-off-by: Gary Kim <gary@garykim.dev>
@wiswedel @rullzer
added line breaks to incoming transfer notification
a1df5d1 
wrapped notifications into double quotes to make line breaks possible

Signed-off-by: Sascha Wiswedel <sascha.wiswedel@nextcloud.com>
@rullzer rullzer force-pushed the wiswedel/transferOwnership/polishing branch from c3bd6aa to a1df5d1 Compare December 17, 2019 09:38
@rullzer
Copy link
Member

rullzer commented Dec 17, 2019

@ChristophWurst fixed with #18438

:)

@gary-kim gary-kim added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Dec 17, 2019
@rullzer rullzer merged commit 4c1c475 into master Dec 17, 2019
@rullzer rullzer deleted the wiswedel/transferOwnership/polishing branch December 17, 2019 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juliusknorr juliusknorr juliusknorr left review comments

@rullzer rullzer rullzer approved these changes

@jancborchardt jancborchardt jancborchardt approved these changes

@ChristophWurst ChristophWurst ChristophWurst approved these changes

Assignees
No one assigned
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement feature: users and groups
Projects
None yet
Milestone
Nextcloud 18.0.0
Development

Successfully merging this pull request may close these issues.
6 participants
@wiswedel @ChristophWurst @jancborchardt @gary-kim @rullzer @juliusknorr