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

Move users management to multi line #17239

Merged
merged 1 commit into from
Dec 12, 2019

Conversation

GretaD
Copy link
Contributor

@GretaD GretaD commented Sep 23, 2019

fix #17208
The user management used to be like this:

user_managn_old

Now it looks like this:

user_mangn_new

This version is in read mode. The toggle to edit the display name will be added with another patch.
@nextcloud/designers what do you think? Any other suggestions related to design?

@GretaD GretaD self-assigned this Sep 23, 2019
@skjnldsv skjnldsv added the design Design, UI, UX, etc. label Sep 23, 2019
@marcoambrosini
Copy link
Member

Looks good to me, I know there's a 'no uppercase' policy @jancborchardt :) but I think that it provides some nice hierarchy here. An alternative to that could be increasing the font size.
The only thing I would do is vertically align those titles (the first is a bit off because of the sub heading).
Also, you're pushing the compiled files, is that on purpose?

@jancborchardt
Copy link
Member

Looks good to me, I know there's a 'no uppercase' policy @jancborchardt :) but I think that it provides some nice hierarchy here. An alternative to that could be increasing the font size.

Hehe, so the point is that the focus should not be on the table headings, but on the content. That is why they were lighter than the content text. The policy and the design does not come out of thin air. ;)

Uppercase text is quite bad for legibility as well as accessibility, and we shouldn’t start using it here on an otherwise unrelated change, especially not in table headings which are not a point of focus. :)

@GretaD could you change the headings to normal case, and a color like color: var(--color-text-maxcontrast);?

@GretaD GretaD force-pushed the feature/17208/Move_usersmanagement_to_multiline branch from 90991aa to 433dd70 Compare November 7, 2019 14:19
@GretaD GretaD requested a review from skjnldsv November 26, 2019 10:56
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.

Rebased and cleaned up :)

  • the menu on the end is not aligned anymore :)
    image
  • Menu not vertically centered
    Capture d’écran_2019-11-26_12-26-39
  • we should not show the password column if it's not in edit mode as nothing is displayed :)
    Capture d’écran_2019-11-26_12-27-38
  • we can hide the userid when editing so only the displayname (editable) is shown in the middle
    Capture d’écran_2019-11-26_12-28-42
  • Some slight mislignment of the multiselects
    Capture d’écran_2019-11-26_12-29-57
  • The quota bar doesn't have the tooltip with details like it had before (in readonly mode)

Otherwise great work!! It's looking very clean and much more understandable :)
👍

@skjnldsv skjnldsv added this to the Nextcloud 18 milestone Nov 26, 2019
autocorrect="off"
autocapitalize="off"
spellcheck="false">
<input
Copy link
Member

@skjnldsv skjnldsv Nov 26, 2019

Choose a reason for hiding this comment

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

  • the inputs submit are not shown anymore :(
    They should still be shown inline and trigger a save :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added back, can you please tell me if its ok now?

@@ -191,6 +214,11 @@
</div>
<div class="userActions">
<div v-if="OC.currentUser !== user.id && user.id !== 'admin' && !loading.all" class="toggleUserActions">
<Actions>
<ActionButton icon="icon-checkmark" @click="editing = false">
Copy link
Member

@skjnldsv skjnldsv Nov 26, 2019

Choose a reason for hiding this comment

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

It just toggle the editing, it doesn't "save", so the text should be something else :)
To save a property you still need to manually click the submit button next to the input you're changing (the multiselects automatically save on change)

Additionally, pressing escape should cancel the editing I think, but that is an additional task ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added "Done" is it better as an option?

computed: {
userGroupsLabels() {
return this.userGroups
.map((groupObject) => groupObject.name)
Copy link
Member

Choose a reason for hiding this comment

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

nice! 👍

@GretaD GretaD force-pushed the feature/17208/Move_usersmanagement_to_multiline branch from f0218b0 to 18f777c Compare November 28, 2019 15:40
@GretaD
Copy link
Contributor Author

GretaD commented Nov 28, 2019

@skjnldsv hey, we were discussing yesterday with Frank and Arthur about the "User Backend" column. Backend is very technical, should we rename it something like Local maybe, instead of Database? Or something else, less technical.

@skjnldsv
Copy link
Member

Backend is very technical

I think it's hidden by default in the bottom left settings ;)

@GretaD
Copy link
Contributor Author

GretaD commented Nov 29, 2019

Backend is very technical

I think it's hidden by default in the bottom left settings ;)

i meant database not backend :) user backend is fine. the database we were discussing to call it smth like Local user or smth

@skjnldsv
Copy link
Member

Yes, it's the "user backend" (database, ldap...) It's hidden by default

@jancborchardt
Copy link
Member

Backend is very technical

I think it's hidden by default in the bottom left settings ;)

i meant database not backend :) user backend is fine. the database we were discussing to call it smth like Local user or smth

To answer your question: Yes, changing the label from "Database" to "Local" seems good! :)

@skjnldsv
Copy link
Member

skjnldsv commented Dec 2, 2019

Ah, sorry I missed your point @GretaD 🙇‍♀️

@GretaD GretaD force-pushed the feature/17208/Move_usersmanagement_to_multiline branch from 50f2f99 to c215919 Compare December 3, 2019 16:00
lib/private/User/Database.php Outdated Show resolved Hide resolved
@GretaD GretaD force-pushed the feature/17208/Move_usersmanagement_to_multiline branch from efc9432 to 375eb3e Compare December 5, 2019 13:18
@GretaD GretaD requested a review from skjnldsv December 5, 2019 13:20
@karlitschek
Copy link
Member

@skjnldsv @GretaD

@skjnldsv
Copy link
Member

skjnldsv commented Dec 8, 2019

What is the state here? Would be great to have this in 18 :-)

hopefully we should be able to get this in yes ;)

@GretaD
Copy link
Contributor Author

GretaD commented Dec 9, 2019

@skjnldsv @GretaD

yes, i will work on tests today and we can have a final review this week.

@jancborchardt
Copy link
Member

Some design review, if that is wanted ;)

users

  • Nice on the username / display name column! 👍
  • The "Password" column is shown but seems useless until you edit a user because it does not contain any info.
  • Table heading only needs a border-bottom of 1px, the 2px is not necessary and we don’t do that elsewhere either.
  • The "Group admin for" table heading wraps too much and looks broken. Should wrap to max 2 lines.
  • Group memberships are not actually shown for me in the overview?
  • Stuff is a bit squished, especially the controls on the right? Having the controls sticky on the right would be much better to have them easily reachable without horizontal scrolling (which is a huge pain)
  • Same for the "Done" button, it could be stickied to the right. If you e.g. just want to edit a person’s email address, you might need to:
    1. Scroll to right
    2. Click edit button
    3. Do the edits
    4. Scroll to right again
    5. Click "Done" button
  • There is some whitespace on the right that seems it should not be there?
    groups whitespace

@GretaD GretaD force-pushed the feature/17208/Move_usersmanagement_to_multiline branch from afe2924 to c9d7a26 Compare December 10, 2019 17:28
And I open the User settings
And I see that the list of users contains the user user0
# disabled because we need the TAB patch:
Scenario: assign user to a group
Copy link
Member

Choose a reason for hiding this comment

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

Please don't use the automated tabs option of phpstorm 🙈

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😭

@rullzer rullzer mentioned this pull request Dec 11, 2019
43 tasks
@GretaD
Copy link
Contributor Author

GretaD commented Dec 11, 2019

* [ ]  The "Password" column is shown but seems useless until you edit a user because it does not contain any info.

Removing it its not that user friendly since you have to wonder where to find the password to change it. We can add a placeholder or text to say smth like " click edit button to change password" or smth

* [ ]  Table heading only needs a border-bottom of 1px, the 2px is not necessary and we don’t do that elsewhere either.

Its border-bottom-width: medium; not border-bottom:2px. I changed it into thin.

* [ ]  The "Group admin for" table heading wraps too much and looks broken. Should wrap to max 2 lines.

This is fixed, can you please check it out again, i cannot reproduce it anymore.

* [ ]  Group memberships are not actually shown for me in the overview?

Yes, thats right, weird bug after the last changes pushed by @skjnldsv, will see whats wrong.

* [ ]  Same for the "Done" button, it could be stickied to the right. If you e.g. just want to edit a person’s email address, you might need to:
  
  1. Scroll to right
  2. Click edit button
  3. Do the edits
  4. Scroll to right again
  5. Click "Done" button

only for mobile, otherwise its fine, you dont need to scroll

* [ ]  There is some whitespace on the right that seems it should not be there?
  ![groups whitespace](https://user-images.githubusercontent.com/925062/70541152-16854380-1b99-11ea-9b9f-1a1572da10c9.png)

this is also fixed, please recheck.

@GretaD GretaD force-pushed the feature/17208/Move_usersmanagement_to_multiline branch 2 times, most recently from 2941ccd to 7208c16 Compare December 11, 2019 22:34
@skjnldsv skjnldsv force-pushed the feature/17208/Move_usersmanagement_to_multiline branch 2 times, most recently from 0fcbf03 to 5f3874a Compare December 12, 2019 11:23
Signed-off-by: Greta Doci <gretadoci@gmail.com>
@skjnldsv skjnldsv force-pushed the feature/17208/Move_usersmanagement_to_multiline branch from 5f3874a to c864bc8 Compare December 12, 2019 11:25
@skjnldsv
Copy link
Member

All rebased and ready to go.
Acceptance tests now passes locally, should be good!

Please review!

@rullzer rullzer mentioned this pull request Dec 12, 2019
34 tasks
@rullzer rullzer 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 12, 2019
@GretaD
Copy link
Contributor Author

GretaD commented Dec 12, 2019

CI all greeeeennn yoohooo

@GretaD GretaD merged commit 42c8f9f into master Dec 12, 2019
@GretaD GretaD deleted the feature/17208/Move_usersmanagement_to_multiline branch December 12, 2019 15:10
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 design Design, UI, UX, etc. feature: users and groups
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move users management to multi line
7 participants