From b05f8ad7e719a167f28889034d3eb1a1ad60adf5 Mon Sep 17 00:00:00 2001 From: ilkin-overleaf <100852799+ilkin-overleaf@users.noreply.github.com> Date: Fri, 18 Aug 2023 12:26:22 +0300 Subject: [PATCH] Merge pull request #14339 from overleaf/ii-filetree-empty-space-click-3 [web] Select project root folder improvements GitOrigin-RevId: 48b80f26adf239215bf04d3db95a61ef35b5cf77 --- .../file-tree/components/file-tree-context.js | 8 +- .../file-tree/components/file-tree-doc.js | 10 +- .../components/file-tree-folder-list.js | 7 +- .../file-tree/components/file-tree-folder.js | 16 +-- .../file-tree/components/file-tree-inner.tsx | 26 ++++ .../file-tree/components/file-tree-root.js | 31 ++--- .../contexts/file-tree-actionable.js | 27 ++-- .../contexts/file-tree-selectable.js | 33 ++--- .../history-file-tree-folder-list.tsx | 31 ++--- .../history/context/history-context.tsx | 6 - .../context/types/history-context-value.ts | 4 - .../components/file-tree-root.test.js | 118 +++++++++++++----- .../file-tree/helpers/render-with-context.js | 3 - 13 files changed, 164 insertions(+), 156 deletions(-) create mode 100644 services/web/frontend/js/features/file-tree/components/file-tree-inner.tsx diff --git a/services/web/frontend/js/features/file-tree/components/file-tree-context.js b/services/web/frontend/js/features/file-tree/components/file-tree-context.js index 7d45259dd1..db1a748dc7 100644 --- a/services/web/frontend/js/features/file-tree/components/file-tree-context.js +++ b/services/web/frontend/js/features/file-tree/components/file-tree-context.js @@ -15,7 +15,6 @@ function FileTreeContext({ reindexReferences, setRefProviderEnabled, setStartedFreeTrial, - setShouldShowVisualSelection, onSelect, children, }) { @@ -24,13 +23,9 @@ function FileTreeContext({ refProviders={refProviders} setRefProviderEnabled={setRefProviderEnabled} setStartedFreeTrial={setStartedFreeTrial} - setShouldShowVisualSelection={setShouldShowVisualSelection} reindexReferences={reindexReferences} > - + {children} @@ -44,7 +39,6 @@ FileTreeContext.propTypes = { refProviders: PropTypes.object.isRequired, setRefProviderEnabled: PropTypes.func.isRequired, setStartedFreeTrial: PropTypes.func.isRequired, - setShouldShowVisualSelection: PropTypes.func.isRequired, onSelect: PropTypes.func.isRequired, children: PropTypes.oneOfType([ PropTypes.arrayOf(PropTypes.node), diff --git a/services/web/frontend/js/features/file-tree/components/file-tree-doc.js b/services/web/frontend/js/features/file-tree/components/file-tree-doc.js index 252c5d1e21..1c7bd54d53 100644 --- a/services/web/frontend/js/features/file-tree/components/file-tree-doc.js +++ b/services/web/frontend/js/features/file-tree/components/file-tree-doc.js @@ -8,16 +8,9 @@ import Icon from '../../../shared/components/icon' import iconTypeFromName from '../util/icon-type-from-name' import classnames from 'classnames' -function FileTreeDoc({ - name, - id, - isFile, - isLinkedFile, - shouldShowVisualSelection, -}) { +function FileTreeDoc({ name, id, isFile, isLinkedFile }) { const { isSelected, props: selectableEntityProps } = useSelectableEntity( id, - shouldShowVisualSelection, isFile ) @@ -45,7 +38,6 @@ FileTreeDoc.propTypes = { id: PropTypes.string.isRequired, isFile: PropTypes.bool, isLinkedFile: PropTypes.bool, - shouldShowVisualSelection: PropTypes.bool, } export const FileTreeIcon = ({ isLinkedFile, name }) => { diff --git a/services/web/frontend/js/features/file-tree/components/file-tree-folder-list.js b/services/web/frontend/js/features/file-tree/components/file-tree-folder-list.js index 7f8c81ec07..7dfe1832de 100644 --- a/services/web/frontend/js/features/file-tree/components/file-tree-folder-list.js +++ b/services/web/frontend/js/features/file-tree/components/file-tree-folder-list.js @@ -12,7 +12,7 @@ function FileTreeFolderList({ classes = {}, dropRef = null, children, - shouldShowVisualSelection, + dataTestId, }) { files = files.map(file => ({ ...file, isFile: true })) const docsAndFiles = [...docs, ...files] @@ -23,6 +23,7 @@ function FileTreeFolderList({ role="tree" ref={dropRef} dnd-container="true" + data-testid={dataTestId} > {folders.sort(compareFunction).map(folder => { return ( @@ -33,7 +34,6 @@ function FileTreeFolderList({ folders={folder.folders} docs={folder.docs} files={folder.fileRefs} - shouldShowVisualSelection={shouldShowVisualSelection} /> ) })} @@ -45,7 +45,6 @@ function FileTreeFolderList({ id={doc._id} isFile={doc.isFile} isLinkedFile={doc.linkedFileData && !!doc.linkedFileData.provider} - shouldShowVisualSelection={shouldShowVisualSelection} /> ) })} @@ -63,7 +62,7 @@ FileTreeFolderList.propTypes = { }), dropRef: PropTypes.func, children: PropTypes.node, - shouldShowVisualSelection: PropTypes.bool, + dataTestId: PropTypes.string, } function compareFunction(one, two) { diff --git a/services/web/frontend/js/features/file-tree/components/file-tree-folder.js b/services/web/frontend/js/features/file-tree/components/file-tree-folder.js index 3514b4bc96..0f319db6f8 100644 --- a/services/web/frontend/js/features/file-tree/components/file-tree-folder.js +++ b/services/web/frontend/js/features/file-tree/components/file-tree-folder.js @@ -14,20 +14,10 @@ import FileTreeItemInner from './file-tree-item/file-tree-item-inner' import FileTreeFolderList from './file-tree-folder-list' import usePersistedState from '../../../shared/hooks/use-persisted-state' -function FileTreeFolder({ - name, - id, - folders, - docs, - files, - shouldShowVisualSelection, -}) { +function FileTreeFolder({ name, id, folders, docs, files }) { const { t } = useTranslation() - const { isSelected, props: selectableEntityProps } = useSelectableEntity( - id, - shouldShowVisualSelection - ) + const { isSelected, props: selectableEntityProps } = useSelectableEntity(id) const { selectedEntityParentIds } = useFileTreeSelectable(id) @@ -97,7 +87,6 @@ function FileTreeFolder({ docs={docs} files={files} dropRef={dropRefList} - shouldShowVisualSelection={shouldShowVisualSelection} /> ) : null} @@ -110,7 +99,6 @@ FileTreeFolder.propTypes = { folders: PropTypes.array.isRequired, docs: PropTypes.array.isRequired, files: PropTypes.array.isRequired, - shouldShowVisualSelection: PropTypes.bool, } export default FileTreeFolder diff --git a/services/web/frontend/js/features/file-tree/components/file-tree-inner.tsx b/services/web/frontend/js/features/file-tree/components/file-tree-inner.tsx new file mode 100644 index 0000000000..3de53a4817 --- /dev/null +++ b/services/web/frontend/js/features/file-tree/components/file-tree-inner.tsx @@ -0,0 +1,26 @@ +import { useFileTreeSelectable } from '../contexts/file-tree-selectable' + +type FileTreeInnerProps = { + children: React.ReactNode +} + +function FileTreeInner({ children }: FileTreeInnerProps) { + const { setIsRootFolderSelected } = useFileTreeSelectable() + + const handleFileTreeClick = () => { + setIsRootFolderSelected(true) + } + + return ( + // eslint-disable-next-line jsx-a11y/click-events-have-key-events, jsx-a11y/no-static-element-interactions +
+ {children} +
+ ) +} + +export default FileTreeInner diff --git a/services/web/frontend/js/features/file-tree/components/file-tree-root.js b/services/web/frontend/js/features/file-tree/components/file-tree-root.js index 51097a2170..51799399fc 100644 --- a/services/web/frontend/js/features/file-tree/components/file-tree-root.js +++ b/services/web/frontend/js/features/file-tree/components/file-tree-root.js @@ -1,4 +1,4 @@ -import React, { useEffect, useState } from 'react' +import React, { useEffect } from 'react' import PropTypes from 'prop-types' import withErrorBoundary from '../../../infrastructure/error-boundary' @@ -18,6 +18,7 @@ import { useDroppable } from '../contexts/file-tree-draggable' import { useFileTreeSocketListener } from '../hooks/file-tree-socket-listener' import FileTreeModalCreateFile from './modals/file-tree-modal-create-file' +import FileTreeInner from './file-tree-inner' const FileTreeRoot = React.memo(function FileTreeRoot({ refProviders, @@ -31,12 +32,6 @@ const FileTreeRoot = React.memo(function FileTreeRoot({ const { _id: projectId } = useProjectContext(projectContextPropTypes) const { fileTreeData } = useFileTreeData() const isReady = projectId && fileTreeData - const [shouldShowVisualSelection, setShouldShowVisualSelection] = - useState(true) - - const handleFileTreeClick = () => { - setShouldShowVisualSelection(false) - } useEffect(() => { if (isReady) onInit() @@ -48,23 +43,15 @@ const FileTreeRoot = React.memo(function FileTreeRoot({ refProviders={refProviders} setRefProviderEnabled={setRefProviderEnabled} setStartedFreeTrial={setStartedFreeTrial} - setShouldShowVisualSelection={setShouldShowVisualSelection} reindexReferences={reindexReferences} onSelect={onSelect} > {isConnected ? null :
} - {/* eslint-disable-next-line jsx-a11y/click-events-have-key-events, jsx-a11y/no-static-element-interactions */} -
- -
+ + + @@ -73,7 +60,7 @@ const FileTreeRoot = React.memo(function FileTreeRoot({ ) }) -function FileTreeRootFolder({ shouldShowVisualSelection }) { +function FileTreeRootFolder() { useFileTreeSocketListener() const { fileTreeData } = useFileTreeData() @@ -89,7 +76,7 @@ function FileTreeRootFolder({ shouldShowVisualSelection }) { classes={{ root: 'file-tree-list' }} dropRef={dropRef} isOver={isOver} - shouldShowVisualSelection={shouldShowVisualSelection} + dataTestId="file-tree-list-root" >
  • @@ -97,10 +84,6 @@ function FileTreeRootFolder({ shouldShowVisualSelection }) { ) } -FileTreeRootFolder.propTypes = { - shouldShowVisualSelection: PropTypes.bool, -} - FileTreeRoot.propTypes = { onSelect: PropTypes.func.isRequired, onInit: PropTypes.func.isRequired, diff --git a/services/web/frontend/js/features/file-tree/contexts/file-tree-actionable.js b/services/web/frontend/js/features/file-tree/contexts/file-tree-actionable.js index 8e98bac22f..f0a66b1488 100644 --- a/services/web/frontend/js/features/file-tree/contexts/file-tree-actionable.js +++ b/services/web/frontend/js/features/file-tree/contexts/file-tree-actionable.js @@ -131,7 +131,7 @@ export function FileTreeActionableProvider({ children }) { ) const { fileTreeData, dispatchRename, dispatchMove } = useFileTreeData() - const { selectedEntityIds } = useFileTreeSelectable() + const { selectedEntityIds, isRootFolderSelected } = useFileTreeSelectable() const [droppedFiles, setDroppedFiles] = useState(null) @@ -263,10 +263,13 @@ export function FileTreeActionableProvider({ children }) { dispatch({ type: ACTION_TYPES.START_CREATE_FOLDER }) }, []) - const parentFolderId = useMemo( - () => getSelectedParentFolderId(fileTreeData, selectedEntityIds), - [fileTreeData, selectedEntityIds] - ) + const parentFolderId = useMemo(() => { + return getSelectedParentFolderId( + fileTreeData, + selectedEntityIds, + isRootFolderSelected + ) + }, [fileTreeData, selectedEntityIds, isRootFolderSelected]) const finishCreatingEntity = useCallback( entity => { @@ -369,8 +372,8 @@ export function FileTreeActionableProvider({ children }) { }, [fileTreeData, projectId, selectedEntityIds]) const value = { - canDelete: selectedEntityIds.size > 0, - canRename: selectedEntityIds.size === 1, + canDelete: selectedEntityIds.size > 0 && !isRootFolderSelected, + canRename: selectedEntityIds.size === 1 && !isRootFolderSelected, canCreate: selectedEntityIds.size < 2, ...state, parentFolderId, @@ -427,7 +430,15 @@ export function useFileTreeActionable() { return context } -function getSelectedParentFolderId(fileTreeData, selectedEntityIds) { +function getSelectedParentFolderId( + fileTreeData, + selectedEntityIds, + isRootFolderSelected +) { + if (isRootFolderSelected) { + return fileTreeData._id + } + // we expect only one entity to be selected in that case, so we pick the first const selectedEntityId = Array.from(selectedEntityIds)[0] if (!selectedEntityId) { diff --git a/services/web/frontend/js/features/file-tree/contexts/file-tree-selectable.js b/services/web/frontend/js/features/file-tree/contexts/file-tree-selectable.js index b37b7f579c..b86a7b86e5 100644 --- a/services/web/frontend/js/features/file-tree/contexts/file-tree-selectable.js +++ b/services/web/frontend/js/features/file-tree/contexts/file-tree-selectable.js @@ -75,11 +75,7 @@ function fileTreeSelectableReadOnlyReducer(selectedEntityIds, action) { } } -export function FileTreeSelectableProvider({ - onSelect, - setShouldShowVisualSelection, - children, -}) { +export function FileTreeSelectableProvider({ onSelect, children }) { const { _id: projectId, rootDocId } = useProjectContext( projectContextPropTypes ) @@ -92,6 +88,8 @@ export function FileTreeSelectableProvider({ const { fileTreeData, setSelectedEntities } = useFileTreeData() + const [isRootFolderSelected, setIsRootFolderSelected] = useState(false) + const [selectedEntityIds, dispatch] = useReducer( permissionsLevel === 'readOnly' ? fileTreeSelectableReadOnlyReducer @@ -181,7 +179,8 @@ export function FileTreeSelectableProvider({ select, unselect, selectOrMultiSelectEntity, - setShouldShowVisualSelection, + isRootFolderSelected, + setIsRootFolderSelected, } return ( @@ -193,7 +192,6 @@ export function FileTreeSelectableProvider({ FileTreeSelectableProvider.propTypes = { onSelect: PropTypes.func.isRequired, - setShouldShowVisualSelection: PropTypes.func.isRequired, children: PropTypes.oneOfType([ PropTypes.arrayOf(PropTypes.node), PropTypes.node, @@ -209,16 +207,13 @@ const editorContextPropTypes = { permissionsLevel: PropTypes.oneOf(['readOnly', 'readAndWrite', 'owner']), } -export function useSelectableEntity( - id, - shouldShowVisualSelection = true, - isFile -) { +export function useSelectableEntity(id, isFile) { const { view, setView } = useLayoutContext(layoutContextPropTypes) const { selectedEntityIds, selectOrMultiSelectEntity, - setShouldShowVisualSelection, + isRootFolderSelected, + setIsRootFolderSelected, } = useContext(FileTreeSelectableContext) const isSelected = selectedEntityIds.has(id) @@ -226,17 +221,11 @@ export function useSelectableEntity( const handleEvent = useCallback( ev => { ev.stopPropagation() - setShouldShowVisualSelection(true) + setIsRootFolderSelected(false) selectOrMultiSelectEntity(id, ev.ctrlKey || ev.metaKey) setView(isFile ? 'file' : 'editor') }, - [ - id, - setShouldShowVisualSelection, - selectOrMultiSelectEntity, - setView, - isFile, - ] + [id, setIsRootFolderSelected, selectOrMultiSelectEntity, setView, isFile] ) const handleClick = useCallback( @@ -266,7 +255,7 @@ export function useSelectableEntity( ) const isVisuallySelected = - shouldShowVisualSelection && isSelected && view !== 'pdf' + !isRootFolderSelected && isSelected && view !== 'pdf' const props = useMemo( () => ({ className: classNames({ selected: isVisuallySelected }), diff --git a/services/web/frontend/js/features/history/components/file-tree/history-file-tree-folder-list.tsx b/services/web/frontend/js/features/history/components/file-tree/history-file-tree-folder-list.tsx index 0a5630bdf9..fb66c42445 100644 --- a/services/web/frontend/js/features/history/components/file-tree/history-file-tree-folder-list.tsx +++ b/services/web/frontend/js/features/history/components/file-tree/history-file-tree-folder-list.tsx @@ -21,21 +21,10 @@ function HistoryFileTreeFolderList({ rootClassName, children, }: HistoryFileTreeFolderListProps) { - const { - selection, - setSelection, - shouldShowVisualSelection, - setShouldShowVisualSelection, - } = useHistoryContext() - - const handleTopLevelClick = () => { - setShouldShowVisualSelection(false) - } + const { selection, setSelection } = useHistoryContext() const handleEvent = useCallback( - (file: FileDiff, event: React.UIEvent) => { - event.stopPropagation() - setShouldShowVisualSelection(true) + (file: FileDiff) => { setSelection(prevSelection => { if (file.pathname !== prevSelection.selectedFile?.pathname) { return { @@ -48,12 +37,12 @@ function HistoryFileTreeFolderList({ return prevSelection }) }, - [setSelection, setShouldShowVisualSelection] + [setSelection] ) const handleClick = useCallback( - (file: FileDiff, event: React.MouseEvent) => { - handleEvent(file, event) + (file: FileDiff) => { + handleEvent(file) }, [handleEvent] ) @@ -61,19 +50,14 @@ function HistoryFileTreeFolderList({ const handleKeyDown = useCallback( (file: FileDiff, event: React.KeyboardEvent) => { if (event.key === 'Enter' || event.key === ' ') { - handleEvent(file, event) + handleEvent(file) } }, [handleEvent] ) return ( - // eslint-disable-next-line jsx-a11y/click-events-have-key-events -
      +
        {folders.map(folder => ( (selectionInitialState) - const [shouldShowVisualSelection, setShouldShowVisualSelection] = - useState(true) const [updatesInfo, setUpdatesInfo] = useState< HistoryContextValue['updatesInfo'] @@ -332,8 +330,6 @@ function useHistory() { setSelection, fetchNextBatchOfUpdates, resetSelection, - shouldShowVisualSelection, - setShouldShowVisualSelection, }), [ loadingFileDiffs, @@ -350,8 +346,6 @@ function useHistory() { setSelection, fetchNextBatchOfUpdates, resetSelection, - shouldShowVisualSelection, - setShouldShowVisualSelection, ] ) diff --git a/services/web/frontend/js/features/history/context/types/history-context-value.ts b/services/web/frontend/js/features/history/context/types/history-context-value.ts index d37966f241..b68b22357b 100644 --- a/services/web/frontend/js/features/history/context/types/history-context-value.ts +++ b/services/web/frontend/js/features/history/context/types/history-context-value.ts @@ -31,8 +31,4 @@ export type HistoryContextValue = { > fetchNextBatchOfUpdates: () => (() => void) | void resetSelection: () => void - shouldShowVisualSelection: boolean - setShouldShowVisualSelection: React.Dispatch< - React.SetStateAction - > } diff --git a/services/web/test/frontend/features/file-tree/components/file-tree-root.test.js b/services/web/test/frontend/features/file-tree/components/file-tree-root.test.js index 4ad0613a36..7932953da1 100644 --- a/services/web/test/frontend/features/file-tree/components/file-tree-root.test.js +++ b/services/web/test/frontend/features/file-tree/components/file-tree-root.test.js @@ -2,6 +2,7 @@ import { expect } from 'chai' import sinon from 'sinon' import { screen, fireEvent, waitFor } from '@testing-library/react' import fetchMock from 'fetch-mock' +import MockedSocket from 'socket.io-mock' import { renderWithEditorContext, @@ -306,37 +307,92 @@ describe('', function () { expect(screen.queryAllByRole('button', { name: 'Menu' })).to.have.length(0) }) - it('deselects files when clicked outside the list but inside wrapping container', function () { - const rootFolder = [ - { - _id: 'root-folder-id', - name: 'rootFolder', - docs: [{ _id: '456def', name: 'main.tex' }], - folders: [], - fileRefs: [], - }, - ] - renderWithEditorContext( - null} - setRefProviderEnabled={() => null} - setStartedFreeTrial={() => null} - onSelect={onSelect} - onInit={onInit} - isConnected - />, - { - rootFolder, - projectId: '123abc', - rootDocId: '456def', - features: {}, - permissionsLevel: 'owner', - } - ) + describe('when deselecting files', function () { + beforeEach(function () { + const rootFolder = [ + { + _id: 'root-folder-id', + name: 'rootFolder', + docs: [{ _id: '123abc', name: 'main.tex' }], + folders: [ + { + _id: '789ghi', + name: 'thefolder', + docs: [{ _id: '456def', name: 'sub.tex' }], + fileRefs: [], + folders: [], + }, + ], + fileRefs: [], + }, + ] + renderWithEditorContext( + null} + setRefProviderEnabled={() => null} + setStartedFreeTrial={() => null} + onSelect={onSelect} + onInit={onInit} + isConnected + />, + { + rootFolder, + projectId: '123abc', + rootDocId: '456def', + features: {}, + permissionsLevel: 'owner', + socket: new MockedSocket(), + } + ) - screen.getByRole('treeitem', { selected: true }) - fireEvent.click(screen.getByTestId('file-tree-inner')) - expect(screen.queryByRole('treeitem', { selected: true })).to.be.null + // select the sub file + const mainDoc = screen.getByRole('treeitem', { name: 'sub.tex' }) + fireEvent.click(mainDoc) + expect(mainDoc.getAttribute('aria-selected')).to.equal('true') + + // click on empty area + fireEvent.click(screen.getByTestId('file-tree-inner')) + }) + + afterEach(function () { + fetchMock.reset() + }) + + it('removes the selected indicator', function () { + expect(screen.queryByRole('treeitem', { selected: true })).to.be.null + }) + + it('disables the "rename" and "delete" buttons', function () { + expect(screen.queryByRole('button', { name: 'Rename' })).to.be.null + expect(screen.queryByRole('button', { name: 'Delete' })).to.be.null + }) + + it('creates new file in the root folder', async function () { + fetchMock.post('express:/project/:projectId/doc', () => 200) + + fireEvent.click(screen.getByRole('button', { name: /new file/i })) + fireEvent.click(screen.getByRole('button', { name: /create/i })) + + const socketData = { + _id: '12345', + name: 'abcdef.tex', + docs: [], + fileRefs: [], + folders: [], + } + window._ide.socket.socketClient.emit( + 'reciveNewDoc', + 'root-folder-id', + socketData + ) + + await fetchMock.flush(true) + + const newItem = screen.getByRole('treeitem', { name: socketData.name }) + const rootEl = screen.getByTestId('file-tree-list-root') + + expect(newItem.parentNode).to.equal(rootEl) + }) }) }) diff --git a/services/web/test/frontend/features/file-tree/helpers/render-with-context.js b/services/web/test/frontend/features/file-tree/helpers/render-with-context.js index 93ab1665a3..4f2a6049c6 100644 --- a/services/web/test/frontend/features/file-tree/helpers/render-with-context.js +++ b/services/web/test/frontend/features/file-tree/helpers/render-with-context.js @@ -24,7 +24,6 @@ export default (children, options = {}) => { setStartedFreeTrial: () => { console.log('started free trial') }, - setShouldShowVisualSelection: () => {}, onSelect: () => {}, ...contextProps, } @@ -33,7 +32,6 @@ export default (children, options = {}) => { reindexReferences, setRefProviderEnabled, setStartedFreeTrial, - setShouldShowVisualSelection, onSelect, ...editorContextProps } = contextProps @@ -43,7 +41,6 @@ export default (children, options = {}) => { reindexReferences={reindexReferences} setRefProviderEnabled={setRefProviderEnabled} setStartedFreeTrial={setStartedFreeTrial} - setShouldShowVisualSelection={setShouldShowVisualSelection} onSelect={onSelect} > {children}