[ui-storageBrowser] Implement storage browser actions #3877
Conversation
nidhibhatg
force-pushed
the
nidhi_sb_actions_m
branch
from
269ae7f to
c48188a
Compare
nidhibhatg
force-pushed
the
nidhi_sb_actions_m
branch
from
c48188a to
04f2d2a
Compare
nidhibhatg
requested review from
bjornalm,
ramprasadagarwal,
JohanAhlen and
tabraiz12
desktop/core/src/desktop/js/apps/storageBrowser/FileChooserModal/FileChooserModal.tsx
Show resolved
Hide resolved
| type: string; | ||
| } | ||
|
|
||
| const FileChooserModal = ({ |
There was a problem hiding this comment.
Like discussed on slack. The name is misleading if the only purpose is to let the user choose a destination path.
There was a problem hiding this comment.
@nidhibhatg Why not use the existing FileChooserModal that's under reactComponents/FileChooser? Could be extended to only show folders and limited to one FS.
| return ( | ||
| <Modal | ||
| cancelText={cancelText} | ||
| className="hue-filechooser-modal cuix antd" |
There was a problem hiding this comment.
do you need the cuix antd classes here?
| ); | ||
|
|
||
| const getColumns = (file: FileChooserTableData) => { | ||
| const columns: ColumnProps<FileChooserTableData>[] = []; |
There was a problem hiding this comment.
Are we not duplicating a lot this behaviour from the StorageBrowserTable.tsx?
In my opinion the StorageBrowserTable.tsx should be more modular, e.g. the actual table in it should be reusable component. I would see if it was possible to create a reusable and configurable "FilesystemTable" that could be used by different components (like this one) instead of duplicating this code. We will also need an actual "file picker" (for the importer and other places) which also needs a "FilesystemTable" to display the files and folders.
|
|
||
| .antd.cuix { | ||
| .hue-filechooser-modal__body{ | ||
| height:350px; |
There was a problem hiding this comment.
Can you please run the lining script to fix all the lint issues?
| jest.clearAllMocks(); | ||
| }); | ||
|
|
||
| test('renders the modal with basic props', async () => { |
There was a problem hiding this comment.
I see two patterns for writing tests,
- test starts with
it('should: majority of the code base follows this (occurs 6167 times across 214 files) - test starts with
test(': in recent times we are following this (occurs 189 times across 22 files)
Are we free to choose any of the above or we are migrating from one pattern to another?
My recommendation would be to stick to 1, as it is bit more readable when reading a code and already majority of codebase following it?
@bjornalm @JohanAhlen your thoughts?
There was a problem hiding this comment.
My vote is for "it('should" since that helps thinking about the actual functionality
| { source_path: selectedFile, destination_path: value }, | ||
| { | ||
| onSuccess: () => { | ||
| setLoadingFiles(false); |
There was a problem hiding this comment.
Can we use the loading state exposed by useSaveData?
| setLoadingFiles(false); | ||
| onSuccessfulAction(); | ||
| }, | ||
| onError: () => { |
There was a problem hiding this comment.
onError also send error as an argument.
onSuccess: onSuccessfulAction,
onError: (renameError: Error) => huePubSub.publish('hue.error', renameError)
There was a problem hiding this comment.
Same can be done for remaining save api calls on this page
| return false; | ||
| } | ||
| const selectedFile = selectedFiles[0]; | ||
| return (isHDFS(selectedFile.path) || isOFS(selectedFile.path)) && selectedFile.type === 'file'; |
There was a problem hiding this comment.
There is an enum BrowserViewType which can be used to selectedFile.type === BrowserViewType.file
enum can be found at desktop/core/src/desktop/js/reactComponents/FileChooser/types.ts
| @@ -140,7 +295,8 @@ const StorageBrowserActions = ({ | |||
| items: getActions(), | |||
| className: 'hue-storage-browser__table-actions-menu' | |||
| }} | |||
| trigger={['click', 'hover']} | |||
| trigger={['click']} | |||
| disabled={getActions().length === 0 ? true : false} | |||
There was a problem hiding this comment.
disabled={getActions().length === 0} should be sufficient
| return false; | ||
| }, [selectedFiles]); | ||
|
|
||
| const getActions = useCallback(() => { |
There was a problem hiding this comment.
can this be used as a variable by converting to useMemo?
| initialValue={selectedFiles[0]?.name} | ||
| /> | ||
| <InputModal | ||
| title={'Setting Replication factor for: ' + selectedFile} |
There was a problem hiding this comment.
title={t('Setting Replication factor for: {{selectedFile}}', {selectedFile})
| onClose={() => setShowCopyModal(false)} | ||
| onSubmit={handleCopy} | ||
| showModal={showCopyModal} | ||
| title="Copy to" |
There was a problem hiding this comment.
Can we use i18n translation here?
Same for other remaining places
| { path: filePath, name: folderName }, | ||
| { | ||
| onSuccess: () => refetchData(), | ||
| onError: () => { |
There was a problem hiding this comment.
Can we handle error in same way as stated earlier?
There was a problem hiding this comment.
@nidhibhatg I am approving this PR to merge it for now, Please take care of the changes in saperate PR.
* [ui-storageBrowser] Improvements on storage browser content summary * [ui-storageBrowser] Update create new files based on new public api * [ui-storageBrowser] Update create new folders based on new public api * [ui-storageBrowser] Update rename to use useSaveData hook * [ui-storageBrowser] Implement UI for set replication * [ui-storageBrowser] Added tests for set replication action * [ui-storageBrowser] Improve storage browser actions using useMemo * [ui-storageBrowser] Implement UI for copy action in storage browser * [ui-storageBrowser] Implement UI for move action in storage browser
) * [ui-storageBrowser] Improvements on storage browser content summary * [ui-storageBrowser] Update create new files based on new public api * [ui-storageBrowser] Update create new folders based on new public api * [ui-storageBrowser] Update rename to use useSaveData hook * [ui-storageBrowser] Implement UI for set replication * [ui-storageBrowser] Added tests for set replication action * [ui-storageBrowser] Improve storage browser actions using useMemo * [ui-storageBrowser] Implement UI for copy action in storage browser * [ui-storageBrowser] Implement UI for move action in storage browser (cherry picked from commit fec209e) (cherry picked from commit 04315413c675571bfe98e430c14355b9fe7eb2df)
* [ui-storageBrowser] Improvements on storage browser content summary * [ui-storageBrowser] Update create new files based on new public api * [ui-storageBrowser] Update create new folders based on new public api * [ui-storageBrowser] Update rename to use useSaveData hook * [ui-storageBrowser] Implement UI for set replication * [ui-storageBrowser] Added tests for set replication action * [ui-storageBrowser] Improve storage browser actions using useMemo * [ui-storageBrowser] Implement UI for copy action in storage browser * [ui-storageBrowser] Implement UI for move action in storage browser
What changes were proposed in this pull request?
How was this patch tested?
Please review Hue Contributing Guide before opening a pull request.