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.

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

feat(tabs): support tab-pane lazy load #1423

Merged
merged 9 commits into from
Oct 26, 2021

Conversation

nooooooom
Copy link
Contributor

close #1374.

@vercel
Copy link

vercel bot commented Oct 22, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/tusimple/naive-ui/6UxV3JF1PSn4wciiPLVqUCEo4ZeP
✅ Preview: https://naive-ui-git-fork-nooooooom-feat-tab-pane-lazy-load-tusimple.vercel.app

@nooooooom
Copy link
Contributor Author

I will add test cases and docs later.

@codecov
Copy link

codecov bot commented Oct 22, 2021

Codecov Report

Merging #1423 (b7d2f79) into main (a41b190) will increase coverage by 0.14%.
The diff coverage is 100.00%.

❗ Current head b7d2f79 differs from pull request most recent head afb0982. Consider uploading reports for the commit afb0982 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1423      +/-   ##
==========================================
+ Coverage   52.89%   53.04%   +0.14%     
==========================================
  Files         534      534              
  Lines       13354    13362       +8     
  Branches     3815     3819       +4     
==========================================
+ Hits         7064     7088      +24     
+ Misses       5164     5145      -19     
- Partials     1126     1129       +3     
Impacted Files Coverage Δ
src/tabs/src/TabPane.tsx 44.44% <ø> (ø)
src/tabs/src/Tabs.tsx 55.68% <100.00%> (+11.17%) ⬆️
src/loading-bar/src/LoadingBarProvider.tsx 70.83% <0.00%> (-4.17%) ⬇️
src/loading-bar/src/LoadingBar.tsx 77.46% <0.00%> (-0.48%) ⬇️
src/log/src/Log.tsx 29.62% <0.00%> (ø)
src/code/src/Code.tsx 70.73% <0.00%> (+1.50%) ⬆️
src/tabs/src/Tab.tsx 33.33% <0.00%> (+2.56%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a41b190...afb0982. Read the comment docs.

slots: {
default: () => [
h(
NTabPane,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Change Tabs.spec.ts to Tabs.spec.tsx to make code tight.

Comment on lines 547 to 562
return tabPaneVNodes.filter((vNode) => {
const props = vNode.props as {
name: string | number
displayDirective: 'show' | 'if' | undefined
'display-directive': 'show' | 'if' | undefined
active: boolean
displayDirective: TabPaneProps['displayDirective']
'display-directive': TabPaneProps['displayDirective']
}
const useVShow = displayDirective === 'show' || _displayDirective === 'show'
const show = value === name
const active = (props.active = props.name === value)
if (vNode.key !== undefined) {
vNode.key = name
}
if (useVShow) {
children.push(withDirectives(vNode, [[vShow, show]]))
} else if (show) {
children.push(vNode)
vNode.key = props.name
}
return (
active ||
(props.displayDirective !== 'if' && props['display-directive'] !== 'if')
)
})
Copy link
Collaborator

@07akioni 07akioni Oct 23, 2021

Choose a reason for hiding this comment

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

active is not a good way to implement the feature.

If you controll whether to show the tab inside TabPane component. Then if there are 10 tabs, there would be 10 TabPane vnode in the first rendering. It would not be a clean implementaion.

The corrrect way is to modify filterMapTabPanes method.

  1. Record activated tab name in a set
  2. Determine whether to show the tab pane in filterMapTabPanes by the set
  3. If set has the name, push it in children, else do nothing. Use v-show for the children in lazy mode.

Copy link
Contributor Author

@nooooooom nooooooom Oct 24, 2021

Choose a reason for hiding this comment

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

Yes, I also had this concern before, which would increase the number of vnode for no reason, so the if is filtered out in subsequent push.

I performed a secondary process in the tab-pane, because I think the better way to save the active state is in the respective components. Maybe I think it will have more complicated usage scenarios so I do so.

I will re-investigate the feature and perform performance testing, and push a better way to deal with it.

@@ -137,6 +144,7 @@ export default defineComponent({
compitableValueRef,
uncontrolledValueRef
)
const renderedNames = reactive(new Set<NonNullable<TabPaneProps['name']>>())
Copy link
Collaborator

Choose a reason for hiding this comment

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

renderedNamesRef

src/tabs/src/Tabs.tsx Show resolved Hide resolved
src/tabs/src/Tabs.tsx Outdated Show resolved Hide resolved
src/tabs/src/Tabs.tsx Outdated Show resolved Hide resolved
src/tabs/src/Tabs.tsx Outdated Show resolved Hide resolved
@nooooooom
Copy link
Contributor Author

😥看来是我把 setup 的处理逻辑记错了,还得继续努力

@07akioni
Copy link
Collaborator

😥看来是我把 setup 的处理逻辑记错了,还得继续努力

加个开发者群么?你可以加一下钉钉二群然后 @ 群主

@nooooooom nooooooom deleted the feat-tab-pane-lazy-load branch November 21, 2021 04:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tabs增加懒加载功能(lazy-load for tabs)
2 participants