Migrate PermissionsEditor to React #4266

gabrieldutra · 2019-10-18T22:35:13Z

What type of PR is this? (check all applicable)

Refactor

Description

Migration of the PermissionsEditor to React.

Make it availble on the Dashboard Page
Get rid of the old code
Review UI/UX

Related Tickets & Documents

--

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

ranbena · 2019-10-19T10:46:17Z

@gabrieldutra I took it for a spin and I don't think the two pane SelectItemsDialog is optimal for this use case - it's confusing and unintuitive. In all other cases using SelectItemsDialog, the selected list and remove action are separate therefore more suitable.

I actually think the original layout works best and perhaps SelectItemsDialog can be fitted to be like this:

Notice I added:

Title change suggestion
Description under title
Help trigger to here
Assuming admins have omni-permission, was added to list with indication.

gabrieldutra · 2019-10-19T17:55:50Z

Thanks @ranbena, I actually had the same thoughts and it was not easy to create a satisfactory flow with the SelectItemsDialog. I also thought about recreating the existing Dialog, but Antd's Select wouldn't support the Avatars.

I have another idea in mind to apply here that I'll explore later today (or tomorrow): Something similar to Google docs's Share Dialog, one dialog for View/Edit/Remove (at some point with a Select for the Permission Type, but for now we can use a Tag) and another SelectItemsDialog only for adding new users. Edit: From a second look, only thing similar to Google is the Dialog with the permission type Select 😅

Regarding your suggestions the only one I have downside in mind is showing the Admins in the list, that can become confusing and not objective when there are many Admins, we better have that implicit :)

arikfr · 2019-10-20T20:00:13Z

I like @ranbena's suggestion. I don't think SelectItemsDialog is a good fit here, because in some cases it's unfeasible to show a list of all users and it's better to let them search.

But I also agree about this one:

Regarding your suggestions the only one I have downside in mind is showing the Admins in the list, that can become confusing and not objective when there are many Admins, we better have that implicit :)

We can add a note about admins having access instead.

ranbena · 2019-10-20T21:16:35Z

but Antd's Select wouldn't support the Avatars.

@gabrieldutra Not sure what you mean. The way QuerySelector is built, you can put any content in the <option>.

We can add a note about admins having access instead.

Sure. Can put it in the dialog description.

gabrieldutra · 2019-10-23T00:00:07Z

Not sure what you mean. The way QuerySelector is built, you can put any content in the <option>.

My bad 😅, I mixed up the value it can have with the Options. The Options really can have ath... So I've updated to be the UI you suggested and now will improve usability from there.

gabrieldutra · 2019-10-26T18:18:20Z

@ranbena I didn't go much further from what the implementation is today, so lmk if you got any quick wins here :)

ranbena

Great work. Got some suggestions and questions.

ranbena · 2019-10-27T05:32:39Z

client/app/components/permissions-editor/PermissionsEditorDialog.jsx

+            renderItem={user => (
+              <List.Item>
+                <UserPreviewCard key={user.id} user={user}>
+                  {user.id === owner.id ? (<Tag>Owner</Tag>) : (


I'd rename to author

Also, reset the .ant-tag right margin so it aligns to the edge.

I'd rename to author

Author does look better, but do you think it can be confusing in code, considering all the other places use owner? (including the backend)

I do that stuff all the time and i think it’s ok as long as it says author only on the presentation layer.

Currently there is no difference between Owner and Author. But it's very possible we will allow changing the user reference here and in this case semantically it becomes more correct to call it Owner, no?

It's not that important. Just making sure users understand the label and I think author is clearer.

ranbena · 2019-10-27T05:42:19Z

client/app/components/permissions-editor/PermissionsEditorDialog.jsx

+  const [grantees, setGrantees] = useState([]);
+  const { loadGrantees, addPermission, removePermission } = useGrantees(aclUrl);
+  const loadUsersWithPermissions = useCallback(() => {
+    setLoadingGrantees(true);


Swapping the list for a loading indicator on each add and remove is quite jarring.
How about adding a small indicator to the top right of the scrollbox?

That was bothering me too, I was waiting for you to say sth about it. I tried a few options like using an indicator in the Search bar, but ended up with more complexity and not satisfactory. Will try that one 👍

ranbena · 2019-10-27T05:44:01Z

client/app/components/permissions-editor/PermissionsEditorDialog.jsx

+    >
+      <UserSelect
+        onSelect={userId => addPermission(userId).then(loadUsersWithPermissions)}
+        previewCardAddon={user => (userHasPermission(user) ? '(already has permission)' : null)}


Wdyt of omitting the already permitted from search results?

- omit users with permissions - remove margin from Tag - use small loading indicator

arikfr

Looks great. See one small bug fix/code suggestion.

client/app/pages/dashboards/dashboard.js

Co-Authored-By: Arik Fraimovich <arik@arikfr.com>

ranbena

Looks great. One layout suggestion.

ranbena · 2019-10-28T05:39:19Z

client/app/components/permissions-editor/PermissionsEditorDialog.jsx

+      <div className="d-flex align-items-center">
+        <h5 className="flex-fill">Users with permissions</h5>
+        {loadingGrantees && <i className="fa fa-spinner fa-pulse" />}
+      </div>
+      <div className="scrollbox p-5 m-t-10" style={{ maxHeight: '40vh' }}>


A small layout suggestion.

Give the "d-flex align-items-center" element m-t-5.

Remove the m-t-10 from scrollbox.

gabrieldutra · 2019-10-29T15:42:56Z

Merging, so I can add it to the Dashboard migration 👌

gabrieldutra added 2 commits October 18, 2019 19:31

Create PermissionsEditor based on SelectItems

80924a9

Merge branch 'master' into react-permissions-editor

ba3f382

weekly-digest bot mentioned this pull request Oct 21, 2019

Weekly Digest (14 October, 2019 - 21 October, 2019) #4271

Closed

gabrieldutra added 2 commits October 21, 2019 07:55

Merge branch 'master' into react-permissions-editor

fec86b0

Create new dialog for PermissionsEditor

86f8d8b

gabrieldutra added 4 commits October 24, 2019 11:01

Merge branch 'master' into react-permissions-editor

36ab2b8

Merge branch 'master' into react-permissions-editor

0d456e5

Cleanup + Dashboard dialog

a7b7a60

Undo changes in SelectItemsDialog

29acc69

gabrieldutra marked this pull request as ready for review October 26, 2019 18:18

gabrieldutra requested a review from ranbena October 26, 2019 18:18

gabrieldutra self-assigned this Oct 26, 2019

ranbena reviewed Oct 27, 2019

View reviewed changes

gabrieldutra added 2 commits October 27, 2019 12:43

CR changes

c32dc7a

- omit users with permissions - remove margin from Tag - use small loading indicator

owner -> author

e295093

arikfr reviewed Oct 27, 2019

View reviewed changes

client/app/pages/dashboards/dashboard.js Outdated Show resolved Hide resolved

query -> dashboard

d9569c2

Co-Authored-By: Arik Fraimovich <arik@arikfr.com>

ranbena approved these changes Oct 28, 2019

View reviewed changes

Margin updates

26032d4

gabrieldutra merged commit 82f488d into master Oct 29, 2019

gabrieldutra deleted the react-permissions-editor branch October 29, 2019 15:43

weekly-digest bot mentioned this pull request Nov 4, 2019

Weekly Digest (28 October, 2019 - 4 November, 2019) #4334

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate PermissionsEditor to React #4266

Migrate PermissionsEditor to React #4266

gabrieldutra commented Oct 18, 2019 •

edited

Loading

ranbena commented Oct 19, 2019 •

edited

Loading

gabrieldutra commented Oct 19, 2019 •

edited

Loading

arikfr commented Oct 20, 2019

ranbena commented Oct 20, 2019

gabrieldutra commented Oct 23, 2019

gabrieldutra commented Oct 26, 2019

ranbena left a comment

ranbena Oct 27, 2019

ranbena Oct 27, 2019

gabrieldutra Oct 27, 2019

ranbena Oct 27, 2019

arikfr Oct 27, 2019

ranbena Oct 28, 2019

ranbena Oct 27, 2019

gabrieldutra Oct 27, 2019

ranbena Oct 27, 2019

arikfr left a comment

ranbena left a comment

ranbena Oct 28, 2019

gabrieldutra commented Oct 29, 2019

Migrate PermissionsEditor to React #4266

Migrate PermissionsEditor to React #4266

Conversation

gabrieldutra commented Oct 18, 2019 • edited Loading

What type of PR is this? (check all applicable)

Description

Related Tickets & Documents

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

ranbena commented Oct 19, 2019 • edited Loading

gabrieldutra commented Oct 19, 2019 • edited Loading

arikfr commented Oct 20, 2019

ranbena commented Oct 20, 2019

gabrieldutra commented Oct 23, 2019

gabrieldutra commented Oct 26, 2019

ranbena left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arikfr left a comment

Choose a reason for hiding this comment

ranbena left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrieldutra commented Oct 29, 2019

gabrieldutra commented Oct 18, 2019 •

edited

Loading

ranbena commented Oct 19, 2019 •

edited

Loading

gabrieldutra commented Oct 19, 2019 •

edited

Loading