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

Remove deprecated usages from antd DropDown, Tooltip, and Tab components #1859

Merged
merged 7 commits into from
Oct 11, 2023
Merged

Remove deprecated usages from antd DropDown, Tooltip, and Tab components #1859

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
45ebb80
fix: remove deprc usage from DependencyGraph
anshgoyalevil Oct 10, 2023
11a4da6
fix: remove deprc overlay from ExternalLinks comp
anshgoyalevil Oct 10, 2023
439a21a
fix: remove deprc usage from AltViewOptions
anshgoyalevil Oct 10, 2023
da228f3
nits
anshgoyalevil Oct 10, 2023
668e120
fix links
anshgoyalevil Oct 10, 2023
c774070
rm unused imports
anshgoyalevil Oct 10, 2023
4337a83
add ExternalLinks example comment
anshgoyalevil Oct 11, 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
27 changes: 15 additions & 12 deletions packages/jaeger-ui/src/components/DependencyGraph/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,6 @@ import { getConfigValue } from '../../utils/config/get-config';
import './index.css';
import withRouteProps from '../../utils/withRouteProps';

const TabPane = Tabs.TabPane;

// export for tests
export const GRAPH_TYPES = {
FORCE_DIRECTED: { type: 'FORCE_DIRECTED', name: 'Force Directed Graph' },
Expand Down Expand Up @@ -92,23 +90,28 @@ export class DependencyGraphPageImpl extends Component {
if (dependencies.length <= dagMaxNumServices) {
GRAPH_TYPE_OPTIONS.push(GRAPH_TYPES.DAG);
}
const tabItems = [];
GRAPH_TYPE_OPTIONS.forEach(opt => {
tabItems.push({
label: opt.name,
key: opt.type,
children: (
<div className="DependencyGraph--graphWrapper">
{opt.type === 'FORCE_DIRECTED' && <DependencyForceGraph nodes={nodes} links={links} />}
{opt.type === 'DAG' && <DAG serviceCalls={dependencies} />}
</div>
),
});
});

return (
<Tabs
onChange={this.handleGraphTypeChange}
activeKey={graphType}
type="card"
tabBarStyle={{ background: '#f5f5f5', padding: '1rem 1rem 0 1rem' }}
>
{GRAPH_TYPE_OPTIONS.map(opt => (
<TabPane className="ub-relelative" tab={opt.name} key={opt.type}>
<div className="DependencyGraph--graphWrapper">
{opt.type === 'FORCE_DIRECTED' && <DependencyForceGraph nodes={nodes} links={links} />}
{opt.type === 'DAG' && <DAG serviceCalls={dependencies} />}
</div>
</TabPane>
))}
</Tabs>
items={tabItems}
/>
);
}
}
Expand Down
13 changes: 5 additions & 8 deletions packages/jaeger-ui/src/components/DependencyGraph/index.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,8 @@
// limitations under the License.

import React from 'react';
import { Tabs } from 'antd';
import { shallow } from 'enzyme';

import DAG from './DAG';
import DependencyForceGraph from './DependencyForceGraph';
import {
DependencyGraphPageImpl as DependencyGraph,
GRAPH_TYPES,
Expand Down Expand Up @@ -78,21 +75,21 @@ describe('<DependencyGraph>', () => {

describe('graph types', () => {
it('renders a menu with options for the graph types', () => {
expect(wrapper.find(Tabs.TabPane).length).toBe(Object.keys(GRAPH_TYPES).length);
expect(wrapper.find({ tab: GRAPH_TYPES.FORCE_DIRECTED.name }).length).toBe(1);
expect(wrapper.find({ tab: GRAPH_TYPES.DAG.name }).length).toBe(1);
expect(wrapper.props().items.length).toBe(Object.keys(GRAPH_TYPES).length);
expect(wrapper.props().items[0].name).toBe(Object.keys(GRAPH_TYPES)[0].name);
expect(wrapper.props().items[1].name).toBe(Object.keys(GRAPH_TYPES)[1].name);
});

it('renders a force graph when FORCE_GRAPH is the selected type', () => {
wrapper.simulate('change', GRAPH_TYPES.FORCE_DIRECTED.type);
expect(wrapper.state('graphType')).toBe(GRAPH_TYPES.FORCE_DIRECTED.type);
expect(wrapper.find(DependencyForceGraph).length).toBe(1);
expect(wrapper.props().activeKey).toBe(GRAPH_TYPES.FORCE_DIRECTED.type);
});

it('renders a DAG graph when DAG is the selected type', () => {
wrapper.simulate('change', GRAPH_TYPES.DAG.type);
expect(wrapper.state('graphType')).toBe(GRAPH_TYPES.DAG.type);
expect(wrapper.find(DAG).length).toBe(1);
expect(wrapper.props().activeKey).toBe(GRAPH_TYPES.DAG.type);
});
});
});
Expand Down
18 changes: 6 additions & 12 deletions packages/jaeger-ui/src/components/TracePage/TracePageHeader/AltViewOptions.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,16 +30,10 @@ describe('AltViewOptions', () => {

let wrapper;
const getLink = text => {
const menu = shallow(wrapper.find(Dropdown).prop('overlay')).dive();
const links = menu.find(Link);
const links = wrapper.find(Dropdown).prop('menu').items;
for (let i = 0; i < links.length; i++) {
const link = links.at(i);
if (link.children().text() === text) return link;
}
const links2 = menu.find('a');
for (let i = 0; i < links2.length; i++) {
const link = links2.at(i);
if (link.children().text() === text) return link;
const link = links[i];
if (link.label.props.children === text) return link.label.props;
}
throw new Error(`Could not find "${text}"`);
};
Expand Down Expand Up @@ -77,11 +71,11 @@ describe('AltViewOptions', () => {

it('tracks viewing JSONs', () => {
expect(trackJsonView).not.toHaveBeenCalled();
getLink('Trace JSON').simulate('click');
getLink('Trace JSON').onClick();
expect(trackJsonView).toHaveBeenCalledTimes(1);

expect(trackRawJsonView).not.toHaveBeenCalled();
getLink('Trace JSON (unadjusted)').simulate('click');
getLink('Trace JSON (unadjusted)').onClick();
expect(trackRawJsonView).toHaveBeenCalledTimes(1);

expect(trackJsonView).toHaveBeenCalledTimes(1);
Expand Down Expand Up @@ -126,7 +120,7 @@ describe('AltViewOptions', () => {
expect(props.onTraceViewChange).toHaveBeenCalledTimes(i);
expect(trackFn).not.toHaveBeenCalled();

getLink(link).simulate('click');
getLink(link).onClick();
expect(props.onTraceViewChange).toHaveBeenCalledTimes(i + 1);
viewInteractions.forEach(({ trackFn: fn }, j) => {
expect(fn).toHaveBeenCalledTimes(j <= i ? 1 : 0);
Expand Down
47 changes: 26 additions & 21 deletions packages/jaeger-ui/src/components/TracePage/TracePageHeader/AltViewOptions.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
// limitations under the License.

import * as React from 'react';
import { Dropdown, Menu, Button } from 'antd';
import { Dropdown, Button } from 'antd';
import { IoChevronDown } from 'react-icons/io5';
import { Link } from 'react-router-dom';
import './AltViewOptions.css';
Expand Down Expand Up @@ -76,17 +76,21 @@ export default function AltViewOptions(props: Props) {
onTraceViewChange(item);
};

const menu = (
<Menu>
{MENU_ITEMS.filter(item => item.viewType !== viewType).map(item => (
<Menu.Item key={item.viewType}>
<a onClick={() => handleSelectView(item.viewType)} role="button">
{item.label}
</a>
</Menu.Item>
))}
{!disableJsonView && (
<Menu.Item>
const dropdownItems = [
...MENU_ITEMS.filter(item => item.viewType !== viewType).map(item => ({
key: item.viewType as ETraceViewType | string,
label: (
<a onClick={() => handleSelectView(item.viewType)} role="button">
Copy link
Member

Choose a reason for hiding this comment

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

so how does this work now? A menu item is a rectangle that should be all clickable, but a link is only clickable where its text is.

Copy link
Member Author

@anshgoyalevil anshgoyalevil Oct 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yes actually. Previously it needed a Menu wrapper for the dropdown overlay, but since v4.24.0 it no longer needs that.
That doesn't mean there is a change in functionality. The items are still clickable all along that rectangle.

Here's a link to the official ant-design documentation which compares both of these usages: Link

{item.label}
</a>
),
})),
];
if (!disableJsonView) {
dropdownItems.push(
{
key: 'trace-json',
label: (
<Link
to={prefixUrl(`/api/traces/${traceID}?prettyPrint=true`)}
rel="noopener noreferrer"
Expand All @@ -95,10 +99,11 @@ export default function AltViewOptions(props: Props) {
>
Trace JSON
</Link>
</Menu.Item>
)}
{!disableJsonView && (
<Menu.Item>
),
},
{
key: 'trace-json-unadjusted',
label: (
<Link
to={prefixUrl(`/api/traces/${traceID}?raw=true&prettyPrint=true`)}
rel="noopener noreferrer"
Expand All @@ -107,15 +112,15 @@ export default function AltViewOptions(props: Props) {
>
Trace JSON (unadjusted)
</Link>
</Menu.Item>
)}
</Menu>
);
),
}
);
}

const currentItem = MENU_ITEMS.find(item => item.viewType === viewType);
const dropdownText = currentItem ? currentItem.label : 'Alternate Views';
return (
<Dropdown overlay={menu}>
<Dropdown menu={{ items: dropdownItems }}>
<Button className="AltViewOptions">
{`${dropdownText} `}
<IoChevronDown />
Expand Down
118 changes: 63 additions & 55 deletions ...ger-ui/src/components/TracePage/TracePageHeader/__snapshots__/AltViewOptions.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,61 +2,69 @@

exports[`AltViewOptions renders correctly 1`] = `
<Dropdown
overlay={
<Menu>
<MenuItem>
<a
onClick={[Function]}
role="button"
>
Trace Graph
</a>
</MenuItem>
<MenuItem>
<a
onClick={[Function]}
role="button"
>
Trace Statistics
</a>
</MenuItem>
<MenuItem>
<a
onClick={[Function]}
role="button"
>
Trace Spans Table
</a>
</MenuItem>
<MenuItem>
<a
onClick={[Function]}
role="button"
>
Trace Flamegraph
</a>
</MenuItem>
<MenuItem>
<Link
onClick={[MockFunction]}
rel="noopener noreferrer"
target="_blank"
to="/api/traces/test trace ID?prettyPrint=true"
>
Trace JSON
</Link>
</MenuItem>
<MenuItem>
<Link
onClick={[MockFunction]}
rel="noopener noreferrer"
target="_blank"
to="/api/traces/test trace ID?raw=true&prettyPrint=true"
>
Trace JSON (unadjusted)
</Link>
</MenuItem>
</Menu>
menu={
Object {
"items": Array [
Object {
"key": "TraceGraph",
"label": <a
onClick={[Function]}
role="button"
>
Trace Graph
</a>,
},
Object {
"key": "TraceStatistics",
"label": <a
onClick={[Function]}
role="button"
>
Trace Statistics
</a>,
},
Object {
"key": "TraceSpansView",
"label": <a
onClick={[Function]}
role="button"
>
Trace Spans Table
</a>,
},
Object {
"key": "TraceFlamegraph",
"label": <a
onClick={[Function]}
role="button"
>
Trace Flamegraph
</a>,
},
Object {
"key": "trace-json",
"label": <Link
onClick={[MockFunction]}
rel="noopener noreferrer"
target="_blank"
to="/api/traces/test trace ID?prettyPrint=true"
>
Trace JSON
</Link>,
},
Object {
"key": "trace-json-unadjusted",
"label": <Link
onClick={[MockFunction]}
rel="noopener noreferrer"
target="_blank"
to="/api/traces/test trace ID?raw=true&prettyPrint=true"
>
Trace JSON (unadjusted)
</Link>,
},
],
}
}
>
<Button
Expand Down
10 changes: 4 additions & 6 deletions packages/jaeger-ui/src/components/common/ExternalLinks.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@

import React from 'react';
import { shallow } from 'enzyme';
import { Menu, Dropdown } from 'antd';
import { Dropdown } from 'antd';

import ExternalLinks from './ExternalLinks';

Expand All @@ -30,13 +30,11 @@ describe('<ExternalLinks>', () => {
const wrapper = shallow(<ExternalLinks links={links} />);
const dropdown = wrapper.find(Dropdown);
expect(dropdown.length).toBe(1);
const linkValues = shallow(dropdown.first().props().overlay).dive();
const submenuItems = linkValues.find(Menu.Item);
const submenuItems = wrapper.props().menu.items;
expect(submenuItems.length).toBe(links.length);
submenuItems.forEach((subMenu, i) => {
const linkValue = subMenu.find('LinkValue');
expect(linkValue.props().href).toBe(links[i].url);
expect(linkValue.props().children).toBe(links[i].text);
expect(subMenu.label.props.href).toBe(links[i].url);
expect(subMenu.label.props.children).toBe(links[i].text);
});
});

Expand Down
Loading