-
Notifications
You must be signed in to change notification settings - Fork 11
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
✨Public and restricted doc editable #357
Conversation
6ed9f15
to
00e7b27
Compare
00e7b27
to
1d1832b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Nice work ! This is a cool feature !
Small observations :
- there is a slight shift in height when the restrictions are displayed or not.
- I might have liked to know that we are on the document as an unconnected guest. Maybe not, I thought about it quickly so I'm sharing it
navigator.clipboard | ||
.writeText(window.location.href) | ||
.then(() => { | ||
toast(t('Link Copied !'), VariantType.SUCCESS, { | ||
duration: 3000, | ||
}); | ||
}) | ||
.catch(() => { | ||
toast(t('Failed to copy link'), VariantType.ERROR, { | ||
duration: 3000, | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be good to turn it into a hook, because I think we could need it in the home page list via the "..." for example. Or even an invitation link for new members instead of inviting one by one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we will need it somewhere else, yes you are right, but we use it only here for the moment, let's keep it scoped until we need it outside.
export enum LinkRole { | ||
READER = 'reader', | ||
EDITOR = 'editor', | ||
} | ||
|
||
export type Base64 = string; | ||
|
||
export interface Doc { | ||
id: string; | ||
title: string; | ||
content: Base64; | ||
link_reach: LinkReach; | ||
link_role: 'reader' | 'editor'; | ||
link_role: LinkRole; | ||
accesses: Access[]; | ||
created_at: string; | ||
updated_at: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this type is not only related to this feature, but can be used everywhere. I moved this file to the root of the feature/docs folder. But maybe in another PR? and maybe. And maybe you don't agree with that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The feature doc-management
actually is for related things about doc that can be used in others features.
{ | ||
label: t('Read only'), | ||
value: LinkRole.READER, | ||
}, | ||
{ | ||
label: t('Can read and edit'), | ||
value: LinkRole.EDITOR, | ||
}, | ||
].map((radio) => ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe create a constant above?
{(doc.link_reach === LinkReach.AUTHENTICATED || | ||
doc.link_reach === LinkReach.PUBLIC) && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you think of creating a constant with explicit naming? like for example :
const showRestrictedOptions = doc.link_reach === LinkReach.AUTHENTICATED || doc.link_reach === LinkReach.PUBLIC
or
const showRestrictedOptions = [LinkReach.AUTHENTICATED, LinkReach.PUBLIC].includes(doc.link_reach);
@@ -164,14 +164,22 @@ test.describe('Document list members', () => { | |||
const shareModal = page.getByLabel('Share modal'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add tests to know if restrictions should be shown or not based on visibility
562cc79
to
2721c4e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Nice work ! This is a cool feature
Yes this feature is waited since the beginning of the project. ^^
navigator.clipboard | ||
.writeText(window.location.href) | ||
.then(() => { | ||
toast(t('Link Copied !'), VariantType.SUCCESS, { | ||
duration: 3000, | ||
}); | ||
}) | ||
.catch(() => { | ||
toast(t('Failed to copy link'), VariantType.ERROR, { | ||
duration: 3000, | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we will need it somewhere else, yes you are right, but we use it only here for the moment, let's keep it scoped until we need it outside.
export enum LinkRole { | ||
READER = 'reader', | ||
EDITOR = 'editor', | ||
} | ||
|
||
export type Base64 = string; | ||
|
||
export interface Doc { | ||
id: string; | ||
title: string; | ||
content: Base64; | ||
link_reach: LinkReach; | ||
link_role: 'reader' | 'editor'; | ||
link_role: LinkRole; | ||
accesses: Access[]; | ||
created_at: string; | ||
updated_at: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The feature doc-management
actually is for related things about doc that can be used in others features.
- change flex property of Box component - Forward the ref of Text component - globalize tooltip padding
We now have 3 visibility options for docs: - public - restricted - authenticated We also have 2 editability options: - readonly - editable The editability options are only available for public and authenticated docs.
2721c4e
to
9c901b5
Compare
Purpose
We increased the visibility options for docs:
We also have 2 editability options:
The editability options are only available for public and authenticated docs.
Proposal
Demo
scrnli_VZHQd70DS9qxw9.webm