Remove deprecated usages from antd DropDown, Tooltip, and Tab components #1859
Conversation
Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>
Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>
Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>
Codecov ReportAll modified lines are covered by tests ✅
📢 Thoughts on this report? Let us know!. |
packages/jaeger-ui/src/components/DependencyGraph/index.test.js
Outdated
Show resolved
Hide resolved
packages/jaeger-ui/src/components/DependencyGraph/index.test.js
Outdated
Show resolved
Hide resolved
| const dropdownItems = [ | ||
| ...MENU_ITEMS.filter(item => item.viewType !== viewType).map(item => ({ | ||
| label: ( | ||
| <a onClick={() => handleSelectView(item.viewType)} role="button"> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
packages/jaeger-ui/src/components/TracePage/TracePageHeader/AltViewOptions.tsx
Outdated
Show resolved
Hide resolved
| })); | ||
|
|
||
| return [{ label: <Menu items={menuItems} />, key: 'external-links' }]; | ||
| }; | ||
|
|
||
| export default function ExternalLinks(props: ExternalLinksProps) { |
There was a problem hiding this comment.
could you please add a comment explaining how this function is used. I can't get it from the context - are these in the top nav or elsewhere?
There was a problem hiding this comment.
Actually, I too couldn't find the actual usage of this component at the front-end UI. Let me try again
There was a problem hiding this comment.
After putting some dummy data, here's what it now looks like, and used to look like before:
Now:
https://github.com/jaegertracing/jaeger-ui/assets/94157520/7f0d84bc-c2fb-488c-9e50-1ec9484ea1e6
Before:
https://github.com/jaegertracing/jaeger-ui/assets/94157520/2395ad60-85b5-4195-9a90-617413a229d9
Seems like a bit of styling has now changed because the dropdown no longer needs Menu wrapper. Making changes really quick.
There was a problem hiding this comment.
What it looks like in latest commit:
tdqx02me.mp4
| export const linkValueList = (links: Link[]) => { | ||
| const menuItems: MenuProps['items'] = links.map(({ text, url }, index) => ({ | ||
| label: <LinkValue href={url}>{text}</LinkValue>, | ||
| key: `${url}-${index}`, |
There was a problem hiding this comment.
I am curious - is there any global uniqueness requirement for these keys in the overall page? If not, why not just use something simpler for a key, like item-${index}?
There was a problem hiding this comment.
I'm not sure about the global uniqueness, but afaik, the scope of uniqueness is limited to the current page that we are at. ${url}-${index} was used previously too, maybe due to any purpose.
Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com> Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>
yurishkuro
added
the
changelog:bugfix-or-minor-feature
Which problem is this PR solving?
Description of the changes
visibleis changed toopenwithout any change in the internal working.How was this change tested?
What next?
Checklist
jaeger:make lint testjaeger-ui:yarn lintandyarn test