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

Jump to bottom

[frontend]Implement pathbrowser in storage browser with improvements #3445

Merged
merged 9 commits into from
Aug 31, 2023
Merged

[frontend]Implement pathbrowser in storage browser with improvements #3445

merged 9 commits into from
Aug 31, 2023

Conversation

nidhibhatg
Copy link
Collaborator

What changes were proposed in this pull request?

  • Allow the PathBrowser breadcrumbs to accept different separators.
  • Add prop for if filesystem icons should be displayed or not.
  • Styling improvements.
  • Tests for pathbrowser component

How was this patch tested?

  • Manually

Please review Hue Contributing Guide before opening a pull request.

bjornalm
bjornalm requested changes Aug 24, 2023
View reviewed changes
Copy link
Collaborator

@bjornalm bjornalm left a comment

Choose a reason for hiding this comment

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

Nice work, see comments. I think we need some more work on especially the styles and the tests.

...ps/storageBrowser/StorageBrowserPage/StorageBrowserTabContents/StorageBrowserTabContent.scss Outdated Show resolved Hide resolved
...pps/storageBrowser/StorageBrowserPage/StorageBrowserTabContents/StorageBrowserTabContent.tsx Outdated Show resolved Hide resolved
...ps/storageBrowser/StorageBrowserPage/StorageBrowserTabContents/StorageBrowserTabContent.scss Outdated Show resolved Hide resolved
desktop/core/src/desktop/js/reactComponents/FileChooser/FileChooserModal/FileChooserModal.tsx Outdated Show resolved Hide resolved
desktop/core/src/desktop/js/reactComponents/FileChooser/PathBrowser/OverflowingItem.scss Outdated Show resolved Hide resolved
desktop/core/src/desktop/js/reactComponents/FileChooser/PathBrowser/PathBrowser.tsx Outdated Show resolved Hide resolved
desktop/core/src/desktop/js/reactComponents/FileChooser/PathBrowser/PathBrowser.test.tsx Outdated Show resolved Hide resolved
desktop/core/src/desktop/js/reactComponents/FileChooser/PathBrowser/PathBrowser.tsx Outdated Show resolved Hide resolved
desktop/core/src/desktop/js/reactComponents/FileChooser/PathBrowser/PathBrowser.tsx Outdated Show resolved Hide resolved
desktop/core/src/desktop/js/reactComponents/FileChooser/PathBrowser/PathBrowser.tsx Outdated Show resolved Hide resolved
bjornalm
bjornalm requested changes Aug 30, 2023
View reviewed changes
desktop/core/src/desktop/js/reactComponents/FileChooser/PathBrowser/Breadcrumb.scss Outdated Show resolved Hide resolved
desktop/core/src/desktop/js/reactComponents/FileChooser/PathBrowser/OverflowingItem.tsx Outdated Show resolved Hide resolved
desktop/core/src/desktop/js/reactComponents/FileChooser/PathBrowser/OverflowingItem.tsx Outdated Show resolved Hide resolved
desktop/core/src/desktop/js/reactComponents/FileChooser/PathBrowser/OverflowingItem.tsx Outdated Show resolved Hide resolved
desktop/core/src/desktop/js/reactComponents/FileChooser/PathBrowser/PathBrowser.scss Outdated Show resolved Hide resolved
desktop/core/src/desktop/js/reactComponents/FileChooser/PathBrowser/PathBrowser.scss Outdated Show resolved Hide resolved
desktop/core/src/desktop/js/reactComponents/FileChooser/PathBrowser/PathBrowser.tsx Outdated Show resolved Hide resolved
tabraiz12
tabraiz12 reviewed Aug 31, 2023
View reviewed changes
...ps/storageBrowser/StorageBrowserPage/StorageBrowserTabContents/StorageBrowserTabContent.scss
display: flex;
margin-top: vars.$fluidx-spacing-xxs;

.hue-storage-browser__filePath {
Copy link
Collaborator

Choose a reason for hiding this comment

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

we are repeating .hue-storage-browser in every class name. even though it is nested. we can just name the class as filePath

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've named it in accordance to BEM

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to nest these classes in .hue-storage-browser-tabContent?

bjornalm
bjornalm approved these changes Aug 31, 2023
View reviewed changes
Copy link
Collaborator

@bjornalm bjornalm left a comment

Choose a reason for hiding this comment

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

Great work. Some minor fixes but then you can merge :-)

...ps/storageBrowser/StorageBrowserPage/StorageBrowserTabContents/StorageBrowserTabContent.scss
display: flex;
margin-top: vars.$fluidx-spacing-xxs;

.hue-storage-browser__filePath {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to nest these classes in .hue-storage-browser-tabContent?

desktop/core/src/desktop/js/reactComponents/FileChooser/PathBrowser/Breadcrumb/Breadcrumb.tsx Outdated Show resolved Hide resolved
desktop/core/src/desktop/js/reactComponents/FileChooser/PathBrowser/Breadcrumb/Breadcrumb.tsx Show resolved Hide resolved
...src/desktop/js/reactComponents/FileChooser/PathBrowser/DropdownMenuItem/DropdownMenuItem.tsx Outdated Show resolved Hide resolved
desktop/core/src/desktop/js/reactComponents/FileChooser/PathBrowser/PathBrowser.tsx
<div
className="hue-path-browser__file-system-icon"
data-testid={`${testId}__file-system-icon`}
>
{icons[extractFileSystem(breadcrumbs[0].label)]}
</div>
) : (
<></>
)}
<div className="hue-path-browser__breadcrumbs" data-testid={`${testId}-breadcrumb`}>
{breadcrumbs.length <= 3 ? (
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: same here, you can use: breadcrumbs.length <= 3 && (

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I believe that the ternary operator is more apt here as breadcrumbs.length <= 3 will render only the breadcrumbs else breadcrumbs with dropdown is rendered

@nidhibhatg nidhibhatg enabled auto-merge (squash) August 31, 2023 14:10
@nidhibhatg nidhibhatg merged commit a903e18 into master Aug 31, 2023
3 checks passed
@nidhibhatg nidhibhatg deleted the nidhi_sb_pb branch August 31, 2023 14:31
tabraiz12 pushed a commit that referenced this pull request Oct 4, 2023
@nidhibhatg
[frontend]Implement pathbrowser in storage browser with improvements (#…
8947423 
…3445)

* [frontend]Implement pathbrowser in storage browser with improvements

(cherry picked from commit a903e18)
wing2fly pushed a commit that referenced this pull request Mar 6, 2024
@nidhibhatg @wing2fly
CDPD-60011: [frontend] Implement pathbrowser in storage browser with …
be0bd7c 
…improvements (#3445)

* [frontend]Implement pathbrowser in storage browser with improvements

(cherry picked from commit a903e18)
(cherry picked from commit 8947423)
Change-Id: I7f60fac61ccf3f2328117f198718c676ea48cd5f
(cherry picked from commit ffb7c87)
athithyaaselvam pushed a commit that referenced this pull request Mar 14, 2024
@nidhibhatg @wing2fly
CDPD-60011: [frontend] Implement pathbrowser in storage browser with …
ffb7c87 
…improvements (#3445)

* [frontend]Implement pathbrowser in storage browser with improvements

(cherry picked from commit a903e18)
(cherry picked from commit 8947423)
Change-Id: I7f60fac61ccf3f2328117f198718c676ea48cd5f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tabraiz12 tabraiz12 tabraiz12 approved these changes

@bjornalm bjornalm bjornalm approved these changes

@JohanAhlen JohanAhlen Awaiting requested review from JohanAhlen

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nidhibhatg @tabraiz12 @bjornalm