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.

Sign up for GitHub

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

#1030 add 'Duplicate document' action in menu #1040

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

#1030 add 'Duplicate document' action in menu #1040

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
66530a8
#1030 add 'Duplicate document' action in menu
florentinap Jun 12, 2024
83723f4
#1030 fix lint warnings
florentinap Jun 13, 2024
46491ba
Merge branch 'main' into duplicate-documet
florentinap Aug 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions app/client/ui/AppUI.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,15 +91,17 @@
}

function pagePanelsHome(owner: IDisposableOwner, appModel: AppModel, app: App) {

Check warning on line 94 in app/client/ui/AppUI.ts

View workflow job for this annotation

GitHub Actions / build_and_test (:lint:python:client:common:smoke:, 18.x, 3.10) 

Trailing spaces not allowed

Check warning on line 94 in app/client/ui/AppUI.ts

View workflow job for this annotation

GitHub Actions / build_and_test (3.11, 18.x, :lint:python:client:common:smoke:stubs:) 

Trailing spaces not allowed
if (shouldShowOnboardingPage(appModel.userPrefsObs)) {
return dom.create(OnboardingPage, appModel);
}

const pageModel = HomeModelImpl.create(owner, appModel, app.clientScope);
const homeModel = HomeModelImpl.create(owner, appModel, app.clientScope);
const pageModel = DocPageModelImpl.create(owner, app, appModel);
const leftPanelOpen = Observable.create(owner, true);

// Set document title to strings like "Home - Grist" or "Org Name - Grist".
owner.autoDispose(subscribe(pageModel.currentPage, pageModel.currentWS, (use, page, ws) => {
owner.autoDispose(subscribe(homeModel.currentPage, homeModel.currentWS, (use, page, ws) => {
const name = (
page === 'trash' ? 'Trash' :
page === 'templates' ? 'Examples & Templates' :
Expand All @@ -114,10 +116,10 @@
panelOpen: leftPanelOpen,
hideOpener: true,
header: dom.create(AppHeader, appModel),
content: createHomeLeftPane(leftPanelOpen, pageModel),
content: createHomeLeftPane(leftPanelOpen, homeModel),
},
headerMain: createTopBarHome(appModel),
contentMain: createDocMenu(pageModel),
contentMain: createDocMenu(homeModel, pageModel),
contentTop: buildHomeBanners(appModel),
testId,
});
Expand Down
61 changes: 38 additions & 23 deletions app/client/ui/DocMenu.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,14 @@
import {loadUserManager} from 'app/client/lib/imports';
import {getTimeFromNow} from 'app/client/lib/timeUtils';
import {reportError} from 'app/client/models/AppModel';
import {docUrl, urlState} from 'app/client/models/gristUrlState';
import {docUrl, getLoginOrSignupUrl, urlState} from 'app/client/models/gristUrlState';
import {DocPageModel} from 'app/client/models/DocPageModel';
import {HomeModel, makeLocalViewSettings, ViewSettings} from 'app/client/models/HomeModel';
import {getWorkspaceInfo, workspaceName} from 'app/client/models/WorkspaceInfo';
import {attachAddNewTip} from 'app/client/ui/AddNewTip';
import * as css from 'app/client/ui/DocMenuCss';
import {buildHomeIntro, buildWorkspaceIntro} from 'app/client/ui/HomeIntro';
import {makeCopy} from 'app/client/ui/MakeCopyMenu';
import {buildUpgradeButton} from 'app/client/ui/ProductUpgrades';
import {buildPinnedDoc, createPinnedDocs} from 'app/client/ui/PinnedDocs';
import {shadowScroll} from 'app/client/ui/shadowScroll';
Expand Down Expand Up @@ -48,13 +50,13 @@
* Usage:
* dom('div', createDocMenu(homeModel))
*/
export function createDocMenu(home: HomeModel): DomElementArg[] {
export function createDocMenu(home: HomeModel, page: DocPageModel): DomElementArg[] {
return [
attachWelcomePopups(home),
dom.domComputed(home.loading, loading => (
loading === 'slow' ? css.spinner(loadingSpinner()) :
loading ? null :
dom.create(createLoadedDocMenu, home)
dom.create(createLoadedDocMenu, home, page)
))
];
}
Expand All @@ -68,7 +70,7 @@
};
}

function createLoadedDocMenu(owner: IDisposableOwner, home: HomeModel) {
function createLoadedDocMenu(owner: IDisposableOwner, home: HomeModel, pageModel: DocPageModel) {
const flashDocId = observable<string|null>(null);
const upgradeButton = buildUpgradeButton(owner, home.app);
return css.docList( /* vbox */
Expand Down Expand Up @@ -109,7 +111,7 @@
// removes all pinned docs when on trash page.
dom.maybe((use) => use(home.currentWSPinnedDocs).length > 0, () => [
css.docListHeader(css.pinnedDocsIcon('PinBig'), t("Pinned Documents")),
createPinnedDocs(home, home.currentWSPinnedDocs),
createPinnedDocs(home, pageModel, home.currentWSPinnedDocs),
]),

// Build the featured templates dom if on the Examples & Templates page.
Expand All @@ -119,7 +121,7 @@
t("Featured"),
testId('featured-templates-header')
),
createPinnedDocs(home, home.featuredTemplates, true),
createPinnedDocs(home, pageModel, home.featuredTemplates, true),
]),

dom.maybe(home.available, () => [
Expand All @@ -134,7 +136,8 @@
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 === 'private' ? 'FolderPrivate' : 'Folder'),

Check failure on line 139 in app/client/ui/DocMenu.ts

View workflow job for this annotation

GitHub Actions / build_and_test (3.11, 18.x, :nbrowser-^[A-G]:) 

Argument of type '"Folder" | "FolderPrivate"' is not assignable to parameter of type 'IconName'.

Check failure on line 139 in app/client/ui/DocMenu.ts

View workflow job for this annotation

GitHub Actions / build_and_test (3.11, 18.x, :nbrowser-^[A-G]:) 

Property 'shareType' does not exist on type 'Workspace'.

Check failure on line 139 in app/client/ui/DocMenu.ts

View workflow job for this annotation

GitHub Actions / build_and_test (3.11, 18.x, :server-2-of-2:) 

Argument of type '"Folder" | "FolderPrivate"' is not assignable to parameter of type 'IconName'.

Check failure on line 139 in app/client/ui/DocMenu.ts

View workflow job for this annotation

GitHub Actions / build_and_test (3.11, 18.x, :server-2-of-2:) 

Property 'shareType' does not exist on type 'Workspace'.

Check failure on line 139 in app/client/ui/DocMenu.ts

View workflow job for this annotation

GitHub Actions / build_and_test (3.11, 18.x, :nbrowser-^[P-S]:) 

Argument of type '"Folder" | "FolderPrivate"' is not assignable to parameter of type 'IconName'.

Check failure on line 139 in app/client/ui/DocMenu.ts

View workflow job for this annotation

GitHub Actions / build_and_test (3.11, 18.x, :nbrowser-^[P-S]:) 

Property 'shareType' does not exist on type 'Workspace'.

Check failure on line 139 in app/client/ui/DocMenu.ts

View workflow job for this annotation

GitHub Actions / build_and_test (3.11, 18.x, :nbrowser-^[^A-S]:) 

Argument of type '"Folder" | "FolderPrivate"' is not assignable to parameter of type 'IconName'.

Check failure on line 139 in app/client/ui/DocMenu.ts

View workflow job for this annotation

GitHub Actions / build_and_test (3.11, 18.x, :nbrowser-^[^A-S]:) 

Property 'shareType' does not exist on type 'Workspace'.

Check failure on line 139 in app/client/ui/DocMenu.ts

View workflow job for this annotation

GitHub Actions / build_and_test (3.11, 18.x, :nbrowser-^[H-L]:) 

Argument of type '"Folder" | "FolderPrivate"' is not assignable to parameter of type 'IconName'.

Check failure on line 139 in app/client/ui/DocMenu.ts

View workflow job for this annotation

GitHub Actions / build_and_test (3.11, 18.x, :nbrowser-^[H-L]:) 

Property 'shareType' does not exist on type 'Workspace'.

Check failure on line 139 in app/client/ui/DocMenu.ts

View workflow job for this annotation

GitHub Actions / build_and_test (3.11, 18.x, :server-1-of-2:) 

Argument of type '"Folder" | "FolderPrivate"' is not assignable to parameter of type 'IconName'.

Check failure on line 139 in app/client/ui/DocMenu.ts

View workflow job for this annotation

GitHub Actions / build_and_test (3.11, 18.x, :server-1-of-2:) 

Property 'shareType' does not exist on type 'Workspace'.

Check failure on line 139 in app/client/ui/DocMenu.ts

View workflow job for this annotation

GitHub Actions / build_and_test (3.11, 18.x, :nbrowser-^[M-O]:) 

Argument of type '"Folder" | "FolderPrivate"' is not assignable to parameter of type 'IconName'.

Check failure on line 139 in app/client/ui/DocMenu.ts

View workflow job for this annotation

GitHub Actions / build_and_test (3.11, 18.x, :nbrowser-^[M-O]:) 

Property 'shareType' does not exist on type 'Workspace'.
workspaceName(home.app, workspace)]
),
testId('doc-header'),
)
Expand All @@ -143,24 +146,24 @@
(page === 'all') ?
dom('div',
showIntro ? buildHomeIntro(home) : null,
buildAllDocsBlock(home, home.workspaces, showIntro, flashDocId, viewSettings),
shouldShowTemplates(home, showIntro) ? buildAllDocsTemplates(home, viewSettings) : null,
buildAllDocsBlock(home, pageModel, home.workspaces, showIntro, flashDocId, viewSettings),
shouldShowTemplates(home, showIntro) ? buildAllDocsTemplates(home, pageModel, viewSettings) : null,

Check warning on line 150 in app/client/ui/DocMenu.ts

View workflow job for this annotation

GitHub Actions / build_and_test (:lint:python:client:common:smoke:, 18.x, 3.10) 

This line has a length of 121. Maximum allowed is 120

Check warning on line 150 in app/client/ui/DocMenu.ts

View workflow job for this annotation

GitHub Actions / build_and_test (3.11, 18.x, :lint:python:client:common:smoke:stubs:) 

This line has a length of 121. Maximum allowed is 120
) :
(page === 'trash') ?
dom('div',
css.docBlock(t("Documents stay in Trash for 30 days, after which they get deleted permanently.")),
dom.maybe((use) => use(home.trashWorkspaces).length === 0, () =>
css.docBlock(t("Trash is empty."))
),
buildAllDocsBlock(home, home.trashWorkspaces, false, flashDocId, viewSettings),
buildAllDocsBlock(home, pageModel, home.trashWorkspaces, false, flashDocId, viewSettings),
) :
(page === 'templates') ?
dom('div',
buildAllTemplates(home, home.templateWorkspaces, viewSettings)
buildAllTemplates(home, pageModel, home.templateWorkspaces, viewSettings)
) :
workspace && !workspace.isSupportWorkspace && workspace.docs?.length ?
css.docBlock(
buildWorkspaceDocBlock(home, workspace, flashDocId, viewSettings),
buildWorkspaceDocBlock(home, pageModel, workspace, flashDocId, viewSettings),
testId('doc-block')
) :
workspace && !workspace.isSupportWorkspace && workspace.docs?.length === 0 ?
Expand All @@ -186,7 +189,7 @@
}

function buildAllDocsBlock(
home: HomeModel, workspaces: Observable<Workspace[]>,
home: HomeModel, page: DocPageModel, workspaces: Observable<Workspace[]>,
showIntro: boolean, flashDocId: Observable<string|null>, viewSettings: ViewSettings,
) {
return dom.forEach(workspaces, (ws) => {
Expand Down Expand Up @@ -218,7 +221,7 @@

testId('ws-header'),
),
buildWorkspaceDocBlock(home, ws, flashDocId, viewSettings),
buildWorkspaceDocBlock(home, page, ws, flashDocId, viewSettings),
testId('doc-block')
);
});
Expand All @@ -230,7 +233,7 @@
*
* If there are no featured templates, builds nothing.
*/
function buildAllDocsTemplates(home: HomeModel, viewSettings: ViewSettings) {
function buildAllDocsTemplates(home: HomeModel, page: DocPageModel, viewSettings: ViewSettings) {
return dom.domComputed(home.featuredTemplates, templates => {
if (templates.length === 0) { return null; }

Expand All @@ -249,7 +252,7 @@
createVideoTourTextButton(),
),
dom.maybe((use) => !use(hideTemplatesObs), () => [
buildTemplateDocs(home, templates, viewSettings),
buildTemplateDocs(home, page, templates, viewSettings),
bigBasicButton(
t("Discover More Templates"),
urlState().setLinkUrl({homePage: 'templates'}),
Expand All @@ -271,7 +274,8 @@
*
* Used on the Examples & Templates below the featured templates.
*/
function buildAllTemplates(home: HomeModel, templateWorkspaces: Observable<Workspace[]>, viewSettings: ViewSettings) {
function buildAllTemplates(home: HomeModel, page: DocPageModel, templateWorkspaces: Observable<Workspace[]>,
viewSettings: ViewSettings) {
return dom.forEach(templateWorkspaces, workspace => {
return css.templatesDocBlock(
css.templateBlockHeader(
Expand All @@ -281,7 +285,7 @@
),
testId('templates-header'),
),
buildTemplateDocs(home, workspace.docs, viewSettings),
buildTemplateDocs(home, page, workspace.docs, viewSettings),
css.docBlock.cls((use) => '-' + use(viewSettings.currentView)),
testId('templates'),
);
Expand Down Expand Up @@ -369,8 +373,8 @@
}


function buildWorkspaceDocBlock(home: HomeModel, workspace: Workspace, flashDocId: Observable<string|null>,
viewSettings: ViewSettings) {
function buildWorkspaceDocBlock(home: HomeModel, page: DocPageModel, workspace: Workspace,
flashDocId: Observable<string|null>, viewSettings: ViewSettings) {
const renaming = observable<Document|null>(null);

function renderDocs(sort: 'date'|'name', view: "list"|"icons") {
Expand All @@ -383,7 +387,7 @@
return dom.forEach(docs, doc => {
if (view === 'icons') {
return dom.update(
buildPinnedDoc(home, doc, workspace),
buildPinnedDoc(home, page, doc, workspace),
testId('doc'),
);
}
Expand Down Expand Up @@ -418,7 +422,7 @@
css.docMenuTrigger(icon('Dots'), testId('doc-options')),
] :
css.docMenuTrigger(icon('Dots'),
menu(() => makeDocOptionsMenu(home, doc, renaming),
menu(() => makeDocOptionsMenu(home, page, doc, renaming),
{placement: 'bottom-start', parentSelectorToMark: '.' + css.docRowWrapper.className}),
// Clicks on the menu trigger shouldn't follow the link that it's contained in.
dom.on('click', (ev) => { ev.stopPropagation(); ev.preventDefault(); }),
Expand Down Expand Up @@ -476,7 +480,8 @@
// losing the doc that was e.g. just renamed.

// Exported because also used by the PinnedDocs component.
export function makeDocOptionsMenu(home: HomeModel, doc: Document, renaming: Observable<Document|null>) {
export function makeDocOptionsMenu(home: HomeModel, page: DocPageModel, doc: Document,
renaming: Observable<Document|null>) {
const org = home.app.currentOrg;
const orgAccess: roles.Role|null = org ? org.access : null;

Expand Down Expand Up @@ -527,6 +532,16 @@
dom.cls('disabled', !roles.canEdit(orgAccess)),
testId('pin-doc')
),
menuItem(() => {
const {appModel} = page;
if (!appModel.currentValidUser) {
page.clearUnsavedChanges();
Copy link
Contributor

Choose a reason for hiding this comment

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

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 clearUnsavedChanges method to the AppModel or TopAppModel
  • Don't create the DocPageModelImpl at all
  • Refactor makeCopy so that uses only AppModel instance (I looked into its code, and it only needs this clearUnsavedChanges method

window.location.href = getLoginOrSignupUrl({srcDocId: urlState().state.get().doc});
return;
}
Comment on lines +537 to +541
Copy link
Collaborator

Choose a reason for hiding this comment

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

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! 👌

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you point me to a situation when this might happen?

return makeCopy({pageModel: page, doc, modalTitle: t("Duplicate Document")});
}, t("Duplicate Document")
),
menuItem(manageUsers, roles.canEditAccess(doc.access) ? t("Manage Users"): t("Access Details"),
testId('doc-access')
)
Expand Down
12 changes: 7 additions & 5 deletions app/client/ui/PinnedDocs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import {getTimeFromNow} from 'app/client/lib/timeUtils';
import {docUrl, urlState} from 'app/client/models/gristUrlState';
import {DocPageModel} from 'app/client/models/DocPageModel';
import {HomeModel} from 'app/client/models/HomeModel';
import {makeDocOptionsMenu, makeRemovedDocOptionsMenu} from 'app/client/ui/DocMenu';
import {transientInput} from 'app/client/ui/transientInput';
Expand All @@ -18,18 +19,19 @@ const testId = makeTestId('test-dm-');
*
* Used only by DocMenu.
*/
export function createPinnedDocs(home: HomeModel, docs: Observable<Document[]>, isExample = false) {
export function createPinnedDocs(home: HomeModel, page: DocPageModel, docs: Observable<Document[]>, isExample = false) {
return pinnedDocList(
dom.forEach(docs, doc => buildPinnedDoc(home, doc, doc.workspace, isExample)),
dom.forEach(docs, doc => buildPinnedDoc(home, page, doc, doc.workspace, isExample)),
testId('pinned-doc-list'),
);
}

/**
* Build a single doc card with a preview and name. A misnomer because it's now used not only for
* pinned docs, but also for the thumnbails (aka "icons") view mode.
* pinned docs, but also for the thumbnails (aka "icons") view mode.
*/
export function buildPinnedDoc(home: HomeModel, doc: Document, workspace: Workspace, isExample = false): HTMLElement {
export function buildPinnedDoc(home: HomeModel, page: DocPageModel, doc: Document, workspace: Workspace,
isExample = false): HTMLElement {
const renaming = observable<Document|null>(null);
const isRenamingDoc = computed((use) => use(renaming) === doc);
return pinnedDocWrapper(
Expand Down Expand Up @@ -82,7 +84,7 @@ export function buildPinnedDoc(home: HomeModel, doc: Document, workspace: Worksp
pinnedDocOptions(icon('Dots'), testId('pinned-doc-options')),
] :
pinnedDocOptions(icon('Dots'),
menu(() => makeDocOptionsMenu(home, doc, renaming),
menu(() => makeDocOptionsMenu(home, page, doc, renaming),
{placement: 'bottom-start'}),
// Clicks on the menu trigger shouldn't follow the link that it's contained in.
dom.on('click', (ev) => { ev.stopPropagation(); ev.preventDefault(); }),
Expand Down
11 changes: 7 additions & 4 deletions app/client/ui/TemplateDocs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import {docUrl, urlState} from 'app/client/models/gristUrlState';
import {theme} from 'app/client/ui2018/cssVars';
import {Document, Workspace} from 'app/common/UserAPI';
import {dom, makeTestId, styled} from 'grainjs';
import {DocPageModel} from 'app/client/models/DocPageModel';
import {HomeModel, ViewSettings} from 'app/client/models/HomeModel';
import * as css from 'app/client/ui/DocMenuCss';
import {buildPinnedDoc} from 'app/client/ui/PinnedDocs';
Expand All @@ -12,7 +13,8 @@ const testId = makeTestId('test-dm-');
/**
* Builds all `templateDocs` according to the specified `viewSettings`.
*/
export function buildTemplateDocs(home: HomeModel, templateDocs: Document[], viewSettings: ViewSettings) {
export function buildTemplateDocs(home: HomeModel, page: DocPageModel, templateDocs: Document[],
viewSettings: ViewSettings) {
const {currentView, currentSort} = viewSettings;
return dom.domComputed((use) => [use(currentView), use(currentSort)] as const, (opts) => {
const [view, sort] = opts;
Expand All @@ -21,7 +23,7 @@ const testId = makeTestId('test-dm-');
if (sort === 'date') {
sortedDocs = sortBy(templateDocs, (d) => d.removedAt || d.updatedAt).reverse();
}
return cssTemplateDocs(dom.forEach(sortedDocs, d => buildTemplateDoc(home, d, d.workspace, view)));
return cssTemplateDocs(dom.forEach(sortedDocs, d => buildTemplateDoc(home, page, d, d.workspace, view)));
});
}

Expand All @@ -34,9 +36,10 @@ const testId = makeTestId('test-dm-');
* If `view` is set to 'icons', the template will be rendered
* as a clickable tile that includes a title, image and description.
*/
function buildTemplateDoc(home: HomeModel, doc: Document, workspace: Workspace, view: 'list'|'icons') {
function buildTemplateDoc(home: HomeModel, page: DocPageModel, doc: Document, workspace: Workspace,
view: 'list'|'icons') {
if (view === 'icons') {
return buildPinnedDoc(home, doc, workspace, true);
return buildPinnedDoc(home, page, doc, workspace, true);
} else {
return css.docRowWrapper(
cssDocRowLink(
Expand Down
Loading