#1047 access signaling using workspace/doc icon #1048

florentinap · 2024-06-14T12:43:22Z

This PR signals access of each workspace and document using their icons. The color of icon is different for each access level:

shared with everyone or anonymous -> icon is green;
shared with other people than yourself (the owner) but not with everybody -> icon is yellow;
shared only with you (the owner)-> icon is gray.

Update: new icons

This PR signals access of each workspace and document using their icons. The color of icon is different for each access level: * shared with everyone or anonymous -> icon is green; * shared with other people than yourself (the owner) but not with everybody -> icon is yellow; * shared only with you (the owner)-> icon is gray.

fflorent

Thanks, it seems to me like this is a good idea, indeed!

I think of color-blind people, which may not see the differences between these colors. I suggest that we could also introduce other icons. What do you think?

florentinap · 2024-06-14T15:20:00Z

Thanks, it seems to me like this is a good idea, indeed!

I think of color-blind people, which may not see the differences between these colors. I suggest that we could also introduce other icons. What do you think?

Sure, I'll do it.

dsagal · 2024-06-19T15:41:02Z

I agree that it is a good idea to private, shared, and link-shared (public) documents, and workspaces. Currently, only link-shared documents are distinguished, with an icon. For comparison, Google Drive distinguishes private vs shared, for both folders and documents, but using a different icon for folders, and adding an icon (similar to ours) for shared docs.

We do need a better design here. In particular, like @fflorent suggested, we should probably distinguish with different icons, rather than with color. Cc'ing @lusebille , in case she can suggest something simple.

Let me also mention an idea that's come up: to allow users to specify an emoji as an icon to identify documents (and maybe workspaces?), similar to how we allow this for pages within documents (see https://support.getgrist.com/page-widgets/#pages). Just noting that as a reason to use a separate icon to indicate access.

lusebille · 2024-06-26T15:05:02Z

Hi I made mocks for this ticket, my proposal is to keep only clear / simple icon ( I've designed some ) , I suggest using only different icons and make folder icons a bit bigger (for be readable ) @fflorent @dsagal @hexaltation

https://www.figma.com/design/wcpetFt6aOKzTszcvPPWLQ/%5B05%2F24%5D-Grist-Design?node-id=294-15776&t=9hvbpy8Fg7eoAurj-1

and the link to icons (design system file )
https://www.figma.com/design/W9k8Usp1xZNvmNc26TpWer/Grist-Design-System?node-id=84-1281&t=g2N35cchwgAhfiVZ-1

dsagal · 2024-06-27T04:28:20Z

Hmm, these look nice and make sense, but Grist already uses the "two-people" icon to represent a link-shared (public) document, and I am a bit worried about changing the meaning of an existing icon.

The second concern is that we had made an intentional choice to use a green icon for publicly-shared documents, to make them stand out since this is a potentially sensitive situation (higher risk of accidental oversharing). With the new design, such documents stand out much less (there is no difference in color or icon placement, only in which icon it is).

@lusebille , what do you think about these concerns?

lusebille · 2024-06-27T14:02:58Z

Hmm, these look nice and make sense, but Grist already uses the "two-people" icon to represent a link-shared (public) document, and I am a bit worried about changing the meaning of an existing icon.

The second concern is that we had made an intentional choice to use a green icon for publicly-shared documents, to make them stand out since this is a potentially sensitive situation (higher risk of accidental oversharing). With the new design, such documents stand out much less (there is no difference in color or icon placement, only in which icon it is).

@lusebille , what do you think about these concerns?

I think I'm not sure to understand correctly the difference of each case ,
if I have to resume a folder/workspace or document can be :

Public ( worldwide , public link outside the organisation)

manage by this ?

Shared with all (inside the organisation)
Share with some people
Not shared ( with no-one)

manage by this ?

I think I may need a clear explanation of all those cases ( we can see that on monday )
for exemple if I'm putting one of my document to 'public' why I'm stiil able to choose 'none' for the inherit access ?

dsagal · 2024-06-28T14:40:50Z

I think I'm not sure to understand correctly the difference of each case , if I have to resume a folder/workspace or document can be :
* Public ( worldwide , public link outside the organisation)

Correct, "public" (also known as "link-shared") gives access to anyone with a link. But it is only available for a document, not for a workspace.

* Shared with all (inside the organisation)

There is no option with this phrasing, although "inherit access" does something similar (more below).

* Share with some people

* Not shared ( with no-one)

This is controlled by which users the document is shared with. If the list in the dialog includes only one owner, then it's private to them, if it includes anyone else, then it's "some people".

manage by this ?

"Inherit access" option works as follows:

For a document, which is always inside a workspace, this option is set to "In full" by default, and says to use the workspace access for this document. I.e. anyone who has access to the document's workspace will have the same level of access to the document.
For a workspace, which is always inside an organization, this option says to use the organization access for the workspace.
Other levels ("View & Edit" and "View Only") tell to use the limit this inherited access to at most Editor role or at most Viewer role. E.g. if a person A is an Owner or Editor on a workspace, and a document inside it is set to "Inherit access: View Only", then the person A would have access to the document, but with the role Viewer.

Inheriting access does not overlap with the "public access" setting, because workspaces don't support "public access", so there is nothing for documents to inherit.

lusebille · 2024-07-01T09:37:10Z

Thanks for the explanation 🙏

So I suggest that (figma link should be ok ) https://www.figma.com/design/wcpetFt6aOKzTszcvPPWLQ/%5B05%2F24%5D-Grist-Design?node-id=294-15776&t=DHxvpRmX7yNUOfCW-1

using the 'unlock' icon instead of 'people' icon for the 'shared with some'
have a different placement between 'public' and the other ones
and if I'm right 'public' in some way is overriding the document initial status so making the initial icon less visible should makes sense
I suggest to be able to see 'shared with other people' infos with a tooltip

dsagal · 2024-07-01T16:26:00Z

Thanks for the explanation 🙏

So I suggest that (figma link should be ok ) https://www.figma.com/design/wcpetFt6aOKzTszcvPPWLQ/%5B05%2F24%5D-Grist-Design?node-id=294-15776&t=DHxvpRmX7yNUOfCW-1
* using the 'unlock' icon instead of 'people' icon for the 'shared with some'

* have a different placement between 'public' and the other ones

* and if I'm right 'public' in some way is overriding the document initial status so making the initial icon less visible should makes sense

* I suggest to be able to see 'shared with other people' infos with a tooltip

I like the earlier design for being clearer and simpler. May we can reduce the possible confusion from changing the meaning of the "people" icon (from meaning "public" to meaning "shared") this way?

For "private", use the lock icon (in grey outline style)
For "shared", use the people icon, but also in grey outline style (rather than green solid style, how it was used previously to mean "public")
For "public", we the world/globe icon, but in green solid style, to match how the people icon was used previously for this purpose, and so that it continues to stand out visually.
Don't show two icons for the same doc/workspace, i.e. if "public", the world/globe should be the only icon. It's too confusing to combine.

For showing details for "shared with other people", I think people would find it helpful, but there are several considerations to keep in mind:

Only someone who has an Owner role has access to this information.
This information has to be fetched, and it's not super quick, so I imagine that fetch would take about a second to show it. It may be better to require a click than just hover, for that reason, and when it takes longer, we'd need to show a spinner.

lusebille · 2024-07-11T15:37:21Z

Ticket is ready from design side https://www.figma.com/design/ze7cIn7QZlMjswn38bdJKE/Ticket-1048?node-id=0-1&t=NcgwSdR6FWOMZGhQ-1 I also joined the icones as svgs
SVG.zip

dsagal · 2024-07-11T16:11:40Z

Thank you @lusebille ! This design looks good, and approved. @florentinap , how do you feel about updating your implementation with the new design?

Applied suggestions from PR; changed icons and remove colors.

fix

fflorent

Thank you @florentinap, I am really looking forward seeing it landed!

fflorent · 2024-09-06T07:39:06Z

app/client/ui/DocMenu.ts

@@ -134,7 +134,7 @@ function createLoadedDocMenu(owner: IDisposableOwner, home: HomeModel) {
                          hasFeaturedTemplates ? t("More Examples and Templates") : t("Examples and Templates")
                      ) :
                      page === 'trash' ? t("Trash") :
-                      workspace && [css.docHeaderIcon('Folder'), workspaceName(home.app, workspace)]
+                      workspace && [css.docHeaderIcon(workspace.shareType === 'public' ? 'Folder' : 'FolderPrivate'), workspaceName(home.app, workspace)]


Could you address the warnings here?

You may use yarn lint to find them (yarn lint:fix to fix those which can be).

fflorent · 2024-09-06T07:49:51Z

app/gen-server/lib/homedb/HomeDBManager.ts

+              roles.getWeakestRole(<roles.Role>user.parentAccess, <roles.Role>maxInheritedRole));
+          });
+          if (permissionDataUsers?.length > 1) {
+            ws.shareType = permissionDataUsers.find((user) => user.email === EVERYONE_EMAIL || user.email === ANONYMOUS_USER_EMAIL || user.id !== scope.userId)


Wouldn't (user) => user.id !== scope.userId be enough? I don't expect a user to connect using the everyone or the anonymous special accounts. Or is there some subtleties that would explain the other conditions?

fflorent · 2024-09-06T08:14:08Z

app/common/UserAPI.ts

@@ -119,6 +119,7 @@ export interface Workspace extends WorkspaceProperties {
  org: Organization;
  orgDomain?: string;
  access: roles.Role;
+  shareType?: string;


I think we could take advantage of TypeScript and that this model is shared between the client and the server to constrain the values of shareType. Either by using an enum or by typing to the expected values ('private' | 'public').

Also I wonder if based on what I see in the code, when the workspace is not private, it should not be considered as shared rather than public (the latest suggesting that it can be viewed even by anonymous users). What do you think?

fflorent · 2024-09-06T08:15:05Z

app/common/UserAPI.ts

@@ -161,6 +162,7 @@ export interface Document extends DocumentProperties {
  id: string;
  workspace: Workspace;
  access: roles.Role;
+  shareType?: string;


Same comment as above regarding the type (enum or using a union the possible values).

fflorent · 2024-09-06T08:17:38Z

app/gen-server/lib/homedb/HomeDBManager.ts

+              ? 'public'
+              : 'private';
+
+            for(const doc of ws.docs) {


Just a bit of styling (sorry, we should have them defined in eslint rules):

Suggested change

for(const doc of ws.docs) {

for (const doc of ws.docs) {

fflorent · 2024-09-06T08:33:05Z

app/gen-server/lib/homedb/HomeDBManager.ts

+                doc.shareType = permissionDataUsersDoc.find((user) => user.email === EVERYONE_EMAIL
+                  || user.email === ANONYMOUS_USER_EMAIL)
+                  ? 'public'
+                  : permissionDataUsersDoc.find((user) => user.id !== scope.userId)
+                    ? 'shared'
+                    : 'private';


I suggest that we may take advantage of a Set and the getAnonymousUserId and getEveryoneUserId to simplify a bit the code:

const userIds = new Set(permissionDataUsersDoc.map(user => user.id)); if (userIds.size === 1 && userIds.has(scope.userId)) { // Note from Florent: I wonder if the second condition is even necessary doc.shareType = 'private'; } else if (userIds.has(this.getEveryoneUserId()) || userIds.has(this.getAnonymousUserId()) { doc.shareType = 'public'; else { doc.shareType = 'shared'; }

fflorent · 2024-09-06T08:40:34Z

app/gen-server/lib/homedb/HomeDBManager.ts

@@ -692,6 +692,36 @@ export class HomeDBManager extends EventEmitter {
          ws.owner = o.owner;
          // Include the org's domain so that the UI can build doc URLs that include the org.
          ws.orgDomain = o.domain;
+
+          // Include shareType based on users permissions to set icon color
+          const {maxInheritedRole, users } =  this.unwrapQueryResult(


Nitpicking for the styling consistency:

Suggested change

const {maxInheritedRole, users } = this.unwrapQueryResult(

const { maxInheritedRole, users } = this.unwrapQueryResult(

fflorent · 2024-09-06T09:09:34Z

app/gen-server/lib/homedb/HomeDBManager.ts

+            return !!roles.getStrongestRole(user.access,
+              roles.getWeakestRole(<roles.Role>user.parentAccess, <roles.Role>maxInheritedRole));


There are much chances that I misunderstand something. Please don't hesitate to tell me if I am wrong :).

I expect this expression roles.getStrongestRole(user.access, roles.getWeakestRole(<roles.Role>user.parentAccess, <roles.Role>maxInheritedRole)); to return the effective role as described in the comment above.

So we filter anyone having at least a direct access or an inherited access.

It would thus be equivalent to:

const hasDirectAccess = Boolean(user.access); const hasInheritedAccess = Boolean(roles.getWeakestRole(<roles.Role>user.parentAccess, <roles.Role>maxInheritedRole))); return hasDirectAccess || hasInheritedAccess;

If so, if you prefer your current implementation, could you add a comment to clarify the that?

fflorent suggested changes Jun 14, 2024

View reviewed changes

gristlabs#1047 fix

3067333

berhalak added the preview Launch preview deployment of this PR label Jun 19, 2024

berhalak removed the preview Launch preview deployment of this PR label Jun 19, 2024

lusebille added -UX/UI preview Launch preview deployment of this PR labels Jun 20, 2024

florentinap and others added 2 commits June 25, 2024 15:15

gristlabs#1047 add new icons for access level

54dcef8

Merge branch 'main' into icon-access

2f9efb0

lusebille added the anct label Jul 11, 2024

gristlabs#1047 access signaling using workspace/doc icon

5b344cb

Applied suggestions from PR; changed icons and remove colors.

florentinap requested a review from fflorent August 28, 2024 07:50

florentinap added 2 commits August 28, 2024 11:40

Merge remote-tracking branch 'upstream/main' into icon-access

d94cc60

gristlabs#1047

090984d

fix

fflorent suggested changes Sep 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#1047 access signaling using workspace/doc icon #1048

#1047 access signaling using workspace/doc icon #1048

florentinap commented Jun 14, 2024 •

edited

Loading

fflorent left a comment

florentinap commented Jun 14, 2024

dsagal commented Jun 19, 2024

lusebille commented Jun 26, 2024 •

edited

Loading

dsagal commented Jun 27, 2024

lusebille commented Jun 27, 2024

dsagal commented Jun 28, 2024

lusebille commented Jul 1, 2024

dsagal commented Jul 1, 2024

lusebille commented Jul 11, 2024 •

edited

Loading

dsagal commented Jul 11, 2024

fflorent left a comment

fflorent Sep 6, 2024

fflorent Sep 6, 2024

fflorent Sep 6, 2024 •

edited

Loading

fflorent Sep 6, 2024

fflorent Sep 6, 2024

fflorent Sep 6, 2024

fflorent Sep 6, 2024

fflorent Sep 6, 2024

	const {maxInheritedRole, users } = this.unwrapQueryResult(
	const { maxInheritedRole, users } = this.unwrapQueryResult(

		return !!roles.getStrongestRole(user.access,
		roles.getWeakestRole(<roles.Role>user.parentAccess, <roles.Role>maxInheritedRole));

#1047 access signaling using workspace/doc icon #1048

Are you sure you want to change the base?

#1047 access signaling using workspace/doc icon #1048

Conversation

florentinap commented Jun 14, 2024 • edited Loading

fflorent left a comment

Choose a reason for hiding this comment

florentinap commented Jun 14, 2024

dsagal commented Jun 19, 2024

lusebille commented Jun 26, 2024 • edited Loading

dsagal commented Jun 27, 2024

lusebille commented Jun 27, 2024

dsagal commented Jun 28, 2024

lusebille commented Jul 1, 2024

dsagal commented Jul 1, 2024

lusebille commented Jul 11, 2024 • edited Loading

dsagal commented Jul 11, 2024

fflorent left a comment

Choose a reason for hiding this comment

fflorent Sep 6, 2024

Choose a reason for hiding this comment

fflorent Sep 6, 2024

Choose a reason for hiding this comment

fflorent Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

fflorent Sep 6, 2024

Choose a reason for hiding this comment

fflorent Sep 6, 2024

Choose a reason for hiding this comment

fflorent Sep 6, 2024

Choose a reason for hiding this comment

fflorent Sep 6, 2024

Choose a reason for hiding this comment

fflorent Sep 6, 2024

Choose a reason for hiding this comment

florentinap commented Jun 14, 2024 •

edited

Loading

lusebille commented Jun 26, 2024 •

edited

Loading

lusebille commented Jul 11, 2024 •

edited

Loading

fflorent Sep 6, 2024 •

edited

Loading