Skip to content

New user button to sidebar - #7600

Merged
rullzer merged 8 commits into
masterfrom
new-user-button-to-sidebar
Feb 28, 2018
Merged

New user button to sidebar#7600
rullzer merged 8 commits into
masterfrom
new-user-button-to-sidebar

Conversation

@skjnldsv

@skjnldsvskjnldsv commented Dec 21, 2017

Copy link
Copy Markdown
Member

Fix#6746
Fix#7162
@nextcloud/designers

capture d ecran_2018-02-23_12-05-25

  • Do we need the confirm submit button?
  • On mobile, clicking the new button hides the sidebar
  • Update documentation

@skjnldsvskjnldsv added 2. developing Work in progress design Design, UI, UX, etc. enhancement feature: users and groups medium papercut Annoying recurring UX issue with possibly simple fix. labels Dec 21, 2017
@skjnldsvskjnldsv self-assigned this Dec 21, 2017
@juliusknorr

Copy link
Copy Markdown
Member

I'm not that sure if we should go with a popover here. I'd rather use the solution @jancborchardt proposed in #7162:

That would add a new row in the content area on top of the table, with empty fields and dropdowns using default values, with the username field focused.

Makes much more sense to enter the new user data inline where it will appear later on as well.

@pixelipo

Copy link
Copy Markdown
Contributor

I agree with @juliushaertl - also because @jancborchardt 's proposed solution would also solve #6468

@skjnldsv

Copy link
Copy Markdown
MemberAuthor

Okay :)

@MariusBluem

Copy link
Copy Markdown
Member

Even if I am the only one maybe, I like the proposed change.

  1. moving the „Add user“ button into the sidebar makes sense, since Mail and Contacts have the button at the same place. It makes the interface more clean and - at least for me - more easy to use. I would even love to see this in the Files UI.

  2. we can discuss the design of the appearing dropdown, there may be place for polishing.

@pixelipo

Copy link
Copy Markdown
Contributor

Nobody is contesting moving the Add button to the sidebar - that is a good solution. It's just that the button shouldn't open a popover, by rather create a new row with input fields in the main container.

It's a UX general rule of thumb that models should be used rarely.

In this particular example it's easy to create a usability issue - lose focus from popover and what happens with data that was already input?

@MariusBluem

Copy link
Copy Markdown
Member

Ah ... okay, sorry for the misunderstanding :)

@MariusBluem

Copy link
Copy Markdown
Member

maybe we should remove the "+add group" button by doing this ... looks strange having to such buttons in the sidebar (with one more prominent than the other) .. "+ add group" is not needed since a group can be created by simply adding an non-existing group to a user. What do you think @nextcloud/designers

@skjnldsv

Copy link
Copy Markdown
MemberAuthor

I agree with @MariusBluem

@rullzerrullzer added this to the Nextcloud 14 milestone Jan 2, 2018
@jancborchardt

Copy link
Copy Markdown
Member

Let's keep separate things separate. :)

This issue is about redoing the "Add new user" flow. This includes adding a button in the sidebar (good start @skjnldsv!), which then adds a new row with inputs at the top of the list like I described and as @juliushaertl referemced above.

What we do with the "Add group" button is separate from this and should not block it.

@skjnldsv
skjnldsvforce-pushed the new-user-button-to-sidebar branch from d600042 to 5b19e31CompareJanuary 8, 2018 04:50
@codecov

codecovBot commented Jan 8, 2018

Copy link
Copy Markdown

Codecov Report

Merging #7600 into master will decrease coverage by <.01%.
The diff coverage is 0%.

@@ Coverage Diff @@## master #7600 +/- ##
===========================================
- Coverage 51.9% 51.9% -0.01% 
Complexity 25434 25434 ===========================================
Files 1609 1609 Lines 95365 95380 +15 Branches 1378 1378 ===========================================
+ Hits 49502 49505 +3 - Misses 45863 45875 +12
Impacted FilesCoverage ΔComplexity Δ
settings/templates/users/main.php0% <ø> (ø)0 <0> (ø)⬇️
settings/templates/users/part.userlist.php0% <0%> (ø)0 <0> (ø)⬇️
settings/templates/users/part.createuser.php0% <0%> (ø)0 <0> (ø)⬇️
apps/files_trashbin/lib/Trashbin.php72.46% <0%> (-0.25%)136% <0%> (ø)
lib/private/Files/Cache/Propagator.php96.2% <0%> (+1.26%)16% <0%> (ø)⬇️
lib/private/Files/ObjectStore/SwiftFactory.php56.32% <0%> (+3.44%)35% <0%> (ø)⬇️

@skjnldsv

skjnldsv commented Jan 8, 2018

Copy link
Copy Markdown
MemberAuthor

capture d ecran_2018-01-08_05-51-26

Okay, pr updated, left to do:

  • Dropdown select off by 2 px
    capture d ecran_2018-01-08_05-50-25

@jancborchardt

jancborchardt commented Jan 9, 2018

Copy link
Copy Markdown
Member

@skjnldsv sorry for the confusion, but the placement is still off. ;)

That would add a new row in the content area on top of the table […]

So it would look like a new row already, above the other user rows. Basically exactly like it is when you edit a user.

That way it's symmetric for people to use, and we don't have to maintain 2 separate interfaces. :)

@skjnldsv

Copy link
Copy Markdown
MemberAuthor

Ahahaha I'm a fool :p
Apparently I forgot how to read 😆

@skjnldsv

skjnldsv commented Jan 9, 2018

Copy link
Copy Markdown
MemberAuthor

Okay, what do you think now? @jancborchardt

capture d ecran_2018-01-09_17-06-26

@jancborchardt

Copy link
Copy Markdown
Member

Looks great! :) Just change the button label from "Create" to "Add user" (as we never use "Create") and it's good.

Please also check @nextcloud/designers :)

@MariusBluem

Copy link
Copy Markdown
Member

b20f7165-235d-47d2-950f-75867f341d73

Why dont you put the „Add User“ arrow in a line together with the 3-dot-menu. This would make room for additional settings like Group Admin, Quota and maybe E-Mail (according to what is shown/enabled) already while adding the user. What do you think? @jancborchardt

@skjnldsv

skjnldsv commented Jan 10, 2018

Copy link
Copy Markdown
MemberAuthor

Why dont you put the „Add User“ arrow in a line together with the 3-dot-menu. This would make room for additional settings like Group Admin, Quota and maybe E-Mail (according to what is shown/enabled) already while adding the user. What do you think? @jancborchardt

I did at first, but we have multiple rows columns hidden. So if the user choose to display all of them (in the settings) the create button goes outside the window :(

@jancborchardt

Copy link
Copy Markdown
Member

@MariusBluem@skjnldsv yeah, I also saw it but came to the conclusion it’s fine. :) In the future we can improve it and show whatever people display, but this is fine for now.

@jancborchardtjancborchardt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@skjnldsv could you make sure the first field »Username« is automatically focused when the button is clicked and row is created?

@jancborchardt

Copy link
Copy Markdown
Member

I fixed the wording to "Add user". :)

@skjnldsv
skjnldsvforce-pushed the new-user-button-to-sidebar branch from a41f629 to 2a5f087CompareJanuary 11, 2018 12:54
@skjnldsvskjnldsv mentioned this pull request Feb 27, 2018
18 tasks
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
USer menu in popover
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Multiselect fixes
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Register menu & copyright
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Allow form and label in popover standard
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
New menu NOT in popover
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Stop autofilling user and password...
SHAME TO THEM: https://bugzilla.mozilla.org/show_bug.cgi?id=956906#c100
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Hide men by default
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Added value and empty check to properly display a confirm button/input
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
w3c html form table compliance and menu fixes
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Various design fixes
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Fix wording to consistent 'Add user'
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
Focus new username input on toggle
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Removed unwanted th after rebase
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
quote fix
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Th to td
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
🙈
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Email input to email type
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Fixed table template cells and fix email input enabling
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Always show email and fixed min-width of name, username, mail and fullname columns
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Use button id
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@rullzer
rullzerforce-pushed the new-user-button-to-sidebar branch from 80dc09c to 042617bCompareFebruary 27, 2018 11:50

@rullzerrullzer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Works as advertised!

@rullzerrullzer added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Feb 27, 2018
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv

Copy link
Copy Markdown
MemberAuthor

capture d ecran_2018-02-27_13-11-11

@skjnldsv

skjnldsv commented Feb 27, 2018

Copy link
Copy Markdown
MemberAuthor

Wait a bit before merging please @rullzer

OKAY now

@MorrisJobke

Copy link
Copy Markdown
Member

Acceptance tests fail:

cenario: log in with invalid user once fixed by admin # /drone/src/github.com/nextcloud/server/tests/acceptance/features/login.feature:32
Given I act as John # ActorContext::iActAs()
And I can not log in with user unknownUser and password 123456acb # LoginPageContext::iCanNotLogInWithUserAndPassword()
When I act as Jane # ActorContext::iActAs()
And I am logged in as the admin # LoginPageContext::iAmLoggedInAsTheAdmin()
And I open the User settings # SettingsMenuContext::iOpenTheUserSettings()
And I create user unknownUser with password 123456acb # UsersSettingsContext::iCreateUserWithPassword()
│ User name field for new user in Users Settings value could not be set
│ Exception message: Element is not currently visible and so may not be interacted with
│ Build info: version: '2.53.1', revision: 'a36b8b1', time: '2016-06-30 17:37:03'
│ System info: host: '47d48347a2f2', ip: '172.17.0.13', os.name: 'Linux', os.arch: 'amd64', os.version: '4.4.0-96-generic', java.version: '1.8.0_91'
│ Driver info: driver.version: unknown
│ Trying again
│
Element is not currently visible and so may not be interacted with

@MorrisJobkeMorrisJobke added 2. developing Work in progress and removed 4. to release Ready to be released and/or waiting for tests to finish labels Feb 27, 2018
@skjnldsv
skjnldsvforce-pushed the new-user-button-to-sidebar branch from 9706b2d to 61f0a9eCompareFebruary 27, 2018 19:40
@skjnldsv

Copy link
Copy Markdown
MemberAuthor
And I set the password for user0 to 654321 # UsersSettingsContext::iSetThePasswordForUserTo()
Offset within element cannot be scrolled into view: (0, 0): [object HTMLTableCellElement]
Build info: version: '2.53.1', revision: 'a36b8b1', time: '2016-06-30 17:37:03'
System info: host: 'a18942a51f1e', ip: '172.17.0.17', os.name: 'Linux', os.arch: 'amd64', os.version: '4.4.0-96-generic', java.version: '1.8.0_91'
Driver info: driver.version: unknown (WebDriver\Exception\MoveTargetOutOfBounds)

Fixed most of it, will see tomorrow if danxuliu did not picked this up before me :)

@MariusBluem

Copy link
Copy Markdown
Member

Will you integrate nextcloud/bookmarks#424 (comment) in this PR?

@skjnldsv

skjnldsv commented Feb 28, 2018

Copy link
Copy Markdown
MemberAuthor

@MariusBluem in a later pr. Do you recall where it was discussed before ? 🤔

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@danxuliu
danxuliuforce-pushed the new-user-button-to-sidebar branch from 3ddfc58 to 35144a7CompareFebruary 28, 2018 10:42
@danxuliu

Copy link
Copy Markdown
Member

The acceptance tests should all work now; I have squashed all the fixes related to the acceptance tests in a single commit.

@skjnldsvskjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 2. developing Work in progress labels Feb 28, 2018
@skjnldsv

Copy link
Copy Markdown
MemberAuthor

Failure unrelated.
Acceptance now passes

@rullzer
rullzer merged commit 926419e into masterFeb 28, 2018
@skjnldsv
skjnldsv deleted the new-user-button-to-sidebar branch February 28, 2018 11:34
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4. to releaseReady to be released and/or waiting for tests to finishdesignDesign, UI, UX, etc.enhancementfeature: users and groupsmediumpapercutAnnoying recurring UX issue with possibly simple fix.standardization

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@skjnldsv@juliusknorr@pixelipo@MariusBluem@jancborchardt@danxuliu@MorrisJobke@rullzer