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

[Workspace] Hide first home breadcrumbs within workspace #7992

Merged
merged 4 commits into from
Sep 5, 2024
Merged

[Workspace] Hide first home breadcrumbs within workspace #7992

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e1c3dff
hide first home breadcrumbs within workspace
Hailong-am Sep 4, 2024
cd0a375
Changeset file for PR #7992 created/updated
opensearch-changeset-bot[bot] Sep 4, 2024
b83d54f
don't relay on text to drop home
Hailong-am Sep 5, 2024
7968674
don't relay on text to drop home
Hailong-am Sep 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions changelogs/fragments/7992.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
feat:
- [Workspace] Hide home breadcrumbs when in a workspace ([#7992](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/7992))
204 changes: 199 additions & 5 deletions src/core/public/chrome/ui/header/__snapshots__/header.test.tsx.snap
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 5 additions & 3 deletions src/core/public/chrome/ui/header/header.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ export function Header({
const [isNavOpen, setIsNavOpen] = useState(false);
const sidecarConfig = useObservable(observables.sidecarConfig$, undefined);
const breadcrumbs = useObservable(observables.breadcrumbs$, []);
const currentWorkspace = useObservable(observables.currentWorkspace$, undefined);

const sidecarPaddingStyle = useMemo(() => {
return getOsdSidecarPaddingStyle(sidecarConfig);
Expand Down Expand Up @@ -203,13 +204,14 @@ export function Header({
/>
);

const renderBreadcrumbs = (renderFullLength?: boolean) => (
const renderBreadcrumbs = (renderFullLength?: boolean, dropHomeFromBreadcrumb?: boolean) => (
<HeaderBreadcrumbs
appTitle$={observables.appTitle$}
breadcrumbs$={observables.breadcrumbs$}
breadcrumbsEnricher$={observables.breadcrumbsEnricher$}
useUpdatedHeader={useUpdatedHeader}
renderFullLength={renderFullLength}
dropHomeFromBreadcrumb={dropHomeFromBreadcrumb}
/>
);

Expand Down Expand Up @@ -351,7 +353,7 @@ export function Header({
recentlyAccessed$={observables.recentlyAccessed$}
workspaceList$={observables.workspaceList$}
navigateToUrl={application.navigateToUrl}
renderBreadcrumbs={renderBreadcrumbs(true)}
renderBreadcrumbs={renderBreadcrumbs(true, false)}
buttonSize={useApplicationHeader ? 's' : 'xs'}
/>
</EuiHeaderSectionItem>
Expand Down Expand Up @@ -399,7 +401,7 @@ export function Header({

<EuiHeaderSection grow={false}>{renderRecentItems()}</EuiHeaderSection>

{renderBreadcrumbs()}
{renderBreadcrumbs(false, !!currentWorkspace)}
</EuiHeader>

{/* Secondary header */}
Expand Down
40 changes: 40 additions & 0 deletions src/core/public/chrome/ui/header/header_breadcrumbs.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,4 +107,44 @@ describe('HeaderBreadcrumbs', () => {
wrapper.update();
expect(wrapper.find('.euiBreadcrumbWrapper')).toHaveLength(2);
});

it('remove heading home when workspace is enabled', () => {
const breadcrumbs$ = new BehaviorSubject([{ text: 'First' }]);
const breadcrumbsEnricher$ = new BehaviorSubject((crumbs: ChromeBreadcrumb[]) => [
{ text: 'Home', home: true },
{ text: 'Analytics' },
...crumbs,
]);
const wrapper = mount(
<HeaderBreadcrumbs
appTitle$={new BehaviorSubject('')}
breadcrumbs$={breadcrumbs$}
breadcrumbsEnricher$={breadcrumbsEnricher$}
dropHomeFromBreadcrumb={true}
/>
);
let breadcrumbs = wrapper.find('.euiBreadcrumbWrapper');
expect(breadcrumbs).toHaveLength(2);
expect(breadcrumbs.at(0).text()).toBe('Analytics');
expect(breadcrumbs.at(1).text()).toBe('First');

// if no home property, we don't drop by text
act(() => {
breadcrumbsEnricher$.next((items) => items);
breadcrumbs$.next([{ text: 'Home' }, { text: 'Second' }]);
});
wrapper.update();
breadcrumbs = wrapper.find('.euiBreadcrumbWrapper');
expect(breadcrumbs).toHaveLength(2);
expect(breadcrumbs.at(0).text()).toBe('Home');
expect(breadcrumbs.at(1).text()).toBe('Second');

act(() => {
breadcrumbsEnricher$.next((items) => []);
breadcrumbs$.next([{ text: 'Home' }, { text: 'Second' }]);
});
wrapper.update();
breadcrumbs = wrapper.find('.euiBreadcrumbWrapper');
expect(breadcrumbs).toHaveLength(0);
});
});
6 changes: 6 additions & 0 deletions src/core/public/chrome/ui/header/header_breadcrumbs.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ interface Props {
breadcrumbsEnricher$: Observable<ChromeBreadcrumbEnricher | undefined>;
useUpdatedHeader?: boolean;
renderFullLength?: boolean;
dropHomeFromBreadcrumb?: boolean;
}

export function HeaderBreadcrumbs({
Expand All @@ -49,6 +50,7 @@ export function HeaderBreadcrumbs({
breadcrumbsEnricher$,
useUpdatedHeader,
renderFullLength,
dropHomeFromBreadcrumb,
}: Props) {
const appTitle = useObservable(appTitle$, 'OpenSearch Dashboards');
const breadcrumbs = useObservable(breadcrumbs$, []);
Expand All @@ -73,6 +75,10 @@ export function HeaderBreadcrumbs({
crumbs = breadcrumbEnricher(crumbs);
}

if (dropHomeFromBreadcrumb && crumbs.length && Object.hasOwn(crumbs[0], 'home')) {
Copy link
Member

@ruanyl ruanyl Sep 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Do we need to check if home is true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we could add this condition as well. There only have one place to set this additional property, i think it's ok only check existence.

Copy link
Member

Choose a reason for hiding this comment

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

Can we have the assumption that when home is set, it must be true?

Copy link
Contributor Author

@Hailong-am Hailong-am Sep 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, i think so. home is not a defined property on ChromeBreadcrumb, ideally nobody else will set this property

crumbs = crumbs.slice(1);
}

crumbs = crumbs.map((breadcrumb, i) => ({
...breadcrumb,
'data-test-subj': classNames(
Expand Down
3 changes: 2 additions & 1 deletion src/plugins/workspace/public/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,8 +385,9 @@ export function prependWorkspaceToBreadcrumbs(
return;
}

const homeBreadcrumb: ChromeBreadcrumb = {
const homeBreadcrumb: ChromeBreadcrumb & { home: boolean } = {
text: 'Home',
home: true,
onClick: () => {
core.application.navigateToApp('home');
},
Expand Down
Loading