-
Notifications
You must be signed in to change notification settings - Fork 365
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] view file content as text #3823
Conversation
desktop/core/src/desktop/js/reactComponents/FileChooser/types.ts
Outdated
Show resolved
Hide resolved
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.
LGTM!
## What changes were proposed in this pull request? - There was no API for getting trash path per user. - Earlier, we were depending on a redirect from backend to the trash path but now having a dedicated API for trash path makes sense for revamping the filebrowser UX. - This API is mainly for HDFS for now because other supported filesystems dont have a clear trash use-case concept in Hue. ## How was this patch tested? - Manually
* [ui-core]: feat: add useFetch hook for API calls * add the license information * fix lint * lint fix * add library * pin the library version * revert extra changes * revert package-lock json * fix: rename hook * fix the function name * typecaste error object
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!
No major remarks, mostly smaller things. Have a look at BEM for class names and our internal standard for test ids.
...esktop/js/apps/storageBrowser/StorageBrowserPage/StorageBrowserTable/StorageBrowserTable.tsx
Show resolved
Hide resolved
...esktop/js/apps/storageBrowser/StorageBrowserPage/StorageBrowserTable/StorageBrowserTable.tsx
Show resolved
Hide resolved
...esktop/js/apps/storageBrowser/StorageBrowserPage/StorageBrowserTable/StorageBrowserTable.tsx
Show resolved
Hide resolved
desktop/core/src/desktop/js/apps/storageBrowser/StorageFilePage/StorageFilePage.scss
Show resolved
Hide resolved
...ps/storageBrowser/StorageBrowserPage/StorageBrowserTabContents/StorageBrowserTabContent.scss
Outdated
Show resolved
Hide resolved
desktop/core/src/desktop/js/apps/storageBrowser/StorageFilePage/StorageFilePage.tsx
Show resolved
Hide resolved
{fileMetaData.map((row, index) => ( | ||
<div key={'meta-data-group' + index} className="group"> | ||
{row.map(item => ( | ||
<div key={item.name} className="column" data-testid="meta-data-attribute"> |
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.
data-testid should uniquely identify an element, here there will be multiple with meta-data-attribute, and it is a bit too generic as well. Don't be afraid to create longer ids see https://github.com/cloudera/hue/blob/master/contributing-frontend.md#adding-data-testid-attribute
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.
It doesn't look like this was resolved
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.
In the final code, I have removed data-testid
as it's not required anymore.
desktop/core/src/desktop/js/apps/storageBrowser/StorageFilePage/StorageFilePage.tsx
Outdated
Show resolved
Hide resolved
desktop/core/src/desktop/js/apps/storageBrowser/StorageFilePage/StorageFilePage.tsx
Show resolved
Hide resolved
desktop/core/src/desktop/js/apps/storageBrowser/StorageFilePage/StorageFilePage.test.tsx
Outdated
Show resolved
Hide resolved
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.
Really nice work, I think you got most of the feedback. Only two comments that I marked as unresolved. You have 38 commits (some well named some not), don't forget to squash once you merge.
What changes were proposed in this pull request?
How was this patch tested?
Screenshots
Please review Hue Contributing Guide before opening a pull request.