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

[CCI] Remove unused tags in the navigation plugin #3964

Merged
merged 3 commits into from
Jun 27, 2023
Merged

[CCI] Remove unused tags in the navigation plugin #3964

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
51c8465
Remove unused tags in the navigation plugin (#3962)
andreymyssak May 2, 2023
dcac27b
Update CHANGELOG.md (#3962)
andreymyssak May 2, 2023
87bb5bb
Merge branch 'main' into 3962-audit-navigation-plugin
manasvinibs Jun 15, 2023
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- [Monaco editor] Add json worker support ([#3424](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3424))
- [Multiple DataSource] Allow create and distinguish index pattern with same name but from different datasources ([#3604](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3604))
- [Multiple DataSource] Integrate multiple datasource with dev tool console ([#3754](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3754))
- [Navigation] Remove unused tags ([#3964](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3964))
- [Notifications] Add id to toast api for deduplication ([#3752](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3752))
- [UI] Add support for comma delimiters in the global filter bar ([#3686](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3686))
- [UI] Indicate that IE is no longer supported ([#3641](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3641))
Expand Down
8 changes: 0 additions & 8 deletions src/plugins/navigation/public/top_nav_menu/top_nav_menu.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ const dataShim = {
};

describe('TopNavMenu', () => {
const WRAPPER_SELECTOR = '.osdTopNavMenu__wrapper';
const TOP_NAV_ITEM_SELECTOR = 'TopNavMenuItem';
const SEARCH_BAR_SELECTOR = 'SearchBar';
const menuItems: TopNavMenuData[] = [
Expand All @@ -66,28 +65,24 @@ describe('TopNavMenu', () => {

it('Should render nothing when no config is provided', () => {
const component = shallowWithIntl(<TopNavMenu appName={'test'} />);
expect(component.find(WRAPPER_SELECTOR).length).toBe(0);
Copy link
Member

Choose a reason for hiding this comment

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

So, it looks like the primary purpose of that class was for unit testing. What do we lose from the integrity of these unit tests by removing the wrapper assertions?

Copy link
Member

Choose a reason for hiding this comment

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

@andreymyssak Can you answer this question? Then I can approve.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joshuarrrr I think we lose nothing in terms of the integrity of the tests. There are only two elements in the component (SearchBar and TopNav) that are meaningful when testing. The WRAPPER_SELECTOR test is not meaningful because it is just a wrapper for the previously mentioned elements, which are tested anyway, whether there is a WRAPPER_SELECTOR or not, so the WRAPPER_SELECTOR test is pretty useless.

expect(component.find(TOP_NAV_ITEM_SELECTOR).length).toBe(0);
expect(component.find(SEARCH_BAR_SELECTOR).length).toBe(0);
});

it('Should not render menu items when config is empty', () => {
const component = shallowWithIntl(<TopNavMenu appName={'test'} config={[]} />);
expect(component.find(WRAPPER_SELECTOR).length).toBe(0);
expect(component.find(TOP_NAV_ITEM_SELECTOR).length).toBe(0);
expect(component.find(SEARCH_BAR_SELECTOR).length).toBe(0);
});

it('Should render 1 menu item', () => {
const component = shallowWithIntl(<TopNavMenu appName={'test'} config={[menuItems[0]]} />);
expect(component.find(WRAPPER_SELECTOR).length).toBe(1);
expect(component.find(TOP_NAV_ITEM_SELECTOR).length).toBe(1);
expect(component.find(SEARCH_BAR_SELECTOR).length).toBe(0);
});

it('Should render multiple menu items', () => {
const component = shallowWithIntl(<TopNavMenu appName={'test'} config={menuItems} />);
expect(component.find(WRAPPER_SELECTOR).length).toBe(1);
expect(component.find(TOP_NAV_ITEM_SELECTOR).length).toBe(menuItems.length);
expect(component.find(SEARCH_BAR_SELECTOR).length).toBe(0);
});
Expand All @@ -96,7 +91,6 @@ describe('TopNavMenu', () => {
const component = shallowWithIntl(
<TopNavMenu appName={'test'} showSearchBar={true} data={dataShim as any} />
);
expect(component.find(WRAPPER_SELECTOR).length).toBe(1);
expect(component.find(TOP_NAV_ITEM_SELECTOR).length).toBe(0);
expect(component.find(SEARCH_BAR_SELECTOR).length).toBe(1);
});
Expand All @@ -105,7 +99,6 @@ describe('TopNavMenu', () => {
const component = shallowWithIntl(
<TopNavMenu appName={'test'} config={menuItems} showSearchBar={true} data={dataShim as any} />
);
expect(component.find(WRAPPER_SELECTOR).length).toBe(1);
expect(component.find(TOP_NAV_ITEM_SELECTOR).length).toBe(menuItems.length);
expect(component.find(SEARCH_BAR_SELECTOR).length).toBe(1);
});
Expand Down Expand Up @@ -170,7 +163,6 @@ describe('TopNavMenu', () => {

await refresh();

expect(component.find(WRAPPER_SELECTOR).length).toBe(1);
expect(component.find(SEARCH_BAR_SELECTOR).length).toBe(1);

// menu is rendered outside of the component
Expand Down
9 changes: 4 additions & 5 deletions src/plugins/navigation/public/top_nav_menu/top_nav_menu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,22 +115,21 @@ export function TopNavMenu(props: TopNavMenuProps): ReactElement | null {
function renderLayout() {
const { setMenuMountPoint } = props;
const menuClassName = classNames('osdTopNavMenu', props.className);
const wrapperClassName = 'osdTopNavMenu__wrapper';
if (setMenuMountPoint) {
return (
<>
<MountPointPortal setMountPoint={setMenuMountPoint}>
<span className={wrapperClassName}>{renderMenu(menuClassName)}</span>
{renderMenu(menuClassName)}
</MountPointPortal>
<span className={wrapperClassName}>{renderSearchBar()}</span>
{renderSearchBar()}
</>
);
} else {
return (
<span className={wrapperClassName}>
<>
{renderMenu(menuClassName)}
{renderSearchBar()}
</span>
</>
);
}
}
Expand Down
Loading