#1030 add 'Duplicate document' action in menu #1040
Conversation
|
Thanks for this enhancement @florentinap! I share below a screenshot of the new feature to help understanding what is the menu item you propose to introduce: |
| if (!appModel.currentValidUser) { | ||
| page.clearUnsavedChanges(); | ||
| window.location.href = getLoginOrSignupUrl({srcDocId: urlState().state.get().doc}); | ||
| return; | ||
| } |
There was a problem hiding this comment.
I wondered if there could be a case where an anonymous user would have access to this menu item, and there is: when they are shared a workspace or an organisation.
In such a case, the workflow is relevant: they are invited to login or to logon. So good catch, that's perfect! 👌
There was a problem hiding this comment.
Can you point me to a situation when this might happen?
|
BTW, I am not able to merge your merge request, only Grist Labs is currently allowed to that (they have tools like internal CI that I don't have access). |
| menuItem(() => { | ||
| const {appModel} = page; | ||
| if (!appModel.currentValidUser) { | ||
| page.clearUnsavedChanges(); |
There was a problem hiding this comment.
This code is not needed, is it? DocPageModel should be only created if we've opened a document. Here we are somehow "lucky" that this constructor isn't doing anything destructive, and is disposed when we open a document (thanks to the internal implementation in gristUrlState.ts/needPageLoad). But still it does create some listeners and computed properties (that do something when url is changed), which is not ideal, and can lead to unexpected bugs later - as, probably, the absence of doc in DocPageModel is rather perceived as temporary state, but here it is enforced.
I think the main culprit here is that clearUnsavedChanges is hold in the DocPageModel and not on the AppModel level, the ideal situation for me would be to refactor it:
- Lift the
clearUnsavedChangesmethod to theAppModelorTopAppModel - Don't create the
DocPageModelImplat all - Refactor
makeCopyso that uses onlyAppModelinstance (I looked into its code, and it only needs thisclearUnsavedChangesmethod
| if (!appModel.currentValidUser) { | ||
| page.clearUnsavedChanges(); | ||
| window.location.href = getLoginOrSignupUrl({srcDocId: urlState().state.get().doc}); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Can you point me to a situation when this might happen?
|
Deployed commit |
|
Now that we can fully preview PRs (thanks @SleepyLeslie!) I played with this. Feels good. Is a little disconcerting that the duplicated document is automatically opened, that makes less sense when duplicating from a workspace listing. |
|
A related issue (#754) led to the same idea, and includes a mock-up: https://www.figma.com/design/wcpetFt6aOKzTszcvPPWLQ/%5B05%2F24%5D-Grist-Design?node-id=1222-73128&t=tM7yRRTAzdKqXnoA-1. The only effect on this PR is to request that the new option be named "Duplicate", and moved up in the menu, between "Move" and "Remove": 
|
This PR adds Duplicate document action in menu of document from workspace page.