-
Notifications
You must be signed in to change notification settings - Fork 133
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.
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
Upgrade antd #811
Upgrade antd #811
Conversation
A kind reminder: I previously found that Tab+Table will have problems, table inside is lazily resized.. Maybe you can try it out and find out how to avoid this issue.. |
Got, let me try it. |
I found this depends on the screen width, the old CardTabs implementation has this problem as well. When the screen is wide, the old implementation is lazily resized at the table end while the new implementation is lazily resized at the first column of the table. (not captured as gif) While the screen is narrow, the old implementation is lazily resized at the first column while the new implementation doesn't resize. When the screen is narrow, the old CardTabs: the new implementation: I guess it may be caused by the container width, will continue to dig it. |
Hard to dig, but I found an workaround, just use the Tab as the indicator, not the container: <ScrollablePane style={{ height: '100vh' }}>
<Card>
<CardTabs
defaultActiveKey={tabKey}
onChange={(key) => {
navigate(`/cluster_info/${key}`)
}}
renderTabBar={renderTabBar}
animated={false}
>
<CardTabs.TabPane
tab={t('cluster_info.list.instance_table.title')}
key="instance"
></CardTabs.TabPane>
<CardTabs.TabPane
tab={t('cluster_info.list.host_table.title')}
key="host"
></CardTabs.TabPane>
<CardTabs.TabPane
tab={t('cluster_info.list.store_topology.title')}
key="store_topology"
></CardTabs.TabPane>
</CardTabs>
{tabKey === 'instance' && <InstanceTable />}
{tabKey === 'host' && <HostTable />}
{tabKey === 'store_topology' && <StoreLocation />}
</Card>
</ScrollablePane> Not sure whether it is fine. |
padding-left: @padding-page; // 48px | ||
height: @padding-page; // 48px | ||
margin-bottom: @padding-md; // 16px | ||
border-bottom: 1px solid @gray-4; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although 1px
and solid
also have variables, I think here using the value may be more direct and explicit.
hi @breeswish PTAL, thanks! |
Fantastic! I will try with this PR later this week. |
To avoid potential problems, I think it's better to check for changes in the changelog of antd. https://github.com/ant-design/ant-design/blob/master/CHANGELOG.zh-CN.md |
I did, but too many 😂, and I noticed that it said the Tabs components have been totally rewritten in 4.3.0
|
Nice dig, can we encapsulate this logic into our own components, in order to avoid writing this hack everywhere? Or is there "detached tabs" available in Antd? |
Let me try it. |
@@ -3,7 +3,7 @@ | |||
margin-right: -@padding-page; | |||
|
|||
:global { | |||
.ant-tabs-bar { | |||
.ant-tabs-nav { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe better to find some way to remove this rule (which depends on the internal implementation of Antd's Tab).
I'm not sure renderTabBar
could be useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, let me try it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works, awesome!
} | ||
|
||
function CardTabs({ className, children, ...restProps }: ICardTabsProps) { | ||
function CardTabs({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hi @breeswish , refactor the CardTabs like this, is it ok?
LGTM |
/merge |
@breeswish: adding 'status/can-merge' to this PR must have 1 LGTMs In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the tidb-community-bots/ti-community-prow repository. |
1 similar comment
@breeswish: adding 'status/can-merge' to this PR must have 1 LGTMs In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the tidb-community-bots/ti-community-prow repository. |
/merge |
Can merge label has been added. Git tree hash: 5c971d3
|
* *: upgrade antd from 4.2.5 to 4.8.5 * yarn upgrade dayjs --latest * fix tabs style * fix tabs style * wip * only use CardTabs as the indicator * use variables in less * refine * refactor CardTabs * refine style Co-authored-by: Wenxuan <hi@breeswish.org>
* ui: increasing precision for metric chart y axis (#823) * build(deps): bump ini from 1.3.5 to 1.3.8 in /ui (#824) * build(deps): bump ini from 1.3.5 to 1.3.8 in /ui/tests (#825) * Upgrade antd (#811) * Upgrade ahooks (#814) * fix issue of query topn statement get error (#827) * ui: show indents for slow query detail time more elegantly (#830) * differ tikv and tiflash nodes (#834) * Release v2021.01.04.1
close #803 , close #711
upgrade antd from 4.2.5 to 4.8.5