-
Notifications
You must be signed in to change notification settings - Fork 371
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
[ui-storageBrowser] Implement storage browser actions #3877
base: master
Are you sure you want to change the base?
Conversation
269ae7f
to
c48188a
Compare
c48188a
to
04f2d2a
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, I have only looked at the FileChooserModal so far, but I think we need some refactoring to get better code reusability and modularity.
{ | ||
params: { | ||
pagesize: '1000', | ||
filter: searchTerm |
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 would apply the URI encoding here instead of when setting the state
type: string; | ||
} | ||
|
||
const FileChooserModal = ({ |
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.
Like discussed on slack. The name is misleading if the only purpose is to let the user choose a destination path.
return ( | ||
<Modal | ||
cancelText={cancelText} | ||
className="hue-filechooser-modal cuix antd" |
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.
do you need the cuix antd
classes here?
); | ||
|
||
const getColumns = (file: FileChooserTableData) => { | ||
const columns: ColumnProps<FileChooserTableData>[] = []; |
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.
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.
What changes were proposed in this pull request?
How was this patch tested?
Please review Hue Contributing Guide before opening a pull request.