feat(virtual-scroll): add virtual-scroll component #1730
Conversation
* fix: 【Charts】update charts snapshots * fix: 修复图表height为100%时的高度问题 * fix: 优化图表extend逻辑 * fix: 修复图表extend失效问题 * fix: 删除注释 * fix: 新增图表echartOption变量，以便用户获取echart配置 * fix: 修复playground百度地图案例错误问题 * fix: 修复图表option复杂变量失效问题 * fix: 修改高德百度地图案例注释 * fix: chart-core添加cloneDeep引入 * fix: 修改chart组件getFormatted函数判断逻辑 * fix: 更新chart-core包版本为3.17.1
* fix(card): 修复card组件样式污染全局问题 * feat(drawer): [drawer] Add drag listening event (opentiny#1701) * feat(drawer): [drawer] Add drag listening event * feat(drawer): [drawer] Modify version number * feat(vue): [button-group] add empty slot (opentiny#1700) * feat(vue): [button-group] add empty slot * docs(button-group): add new api version * fix(select): fix Select‘s SMB theme, multi select svg color (opentiny#1705) * feat: update theme version * fix(popper): fix build error in vitepress (opentiny#1711) * fix(popper): fix build error in vitepress * fix(popper): 升级版本 * fix(select): rename select's css var name (opentiny#1713) --------- Co-authored-by: zzcr <894103554@qq.com> Co-authored-by: MomoPoppy <125256456+MomoPoppy@users.noreply.github.com> Co-authored-by: gimmyhehe <975402925@qq.com> Co-authored-by: 申君健 <40288193@qq.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes introduce a robust virtual scrolling feature to the Vue project, enhancing component interactions and overall performance. Key updates include the addition of the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as UI Component
participant VirtualScroll as VirtualScroll Component
participant Renderless as Renderless Logic
User ->> UI: Scrolls Content
UI ->> VirtualScroll: Update Scroll Position
VirtualScroll ->> Renderless: Fetch Data
Renderless -->> VirtualScroll: Return Data
VirtualScroll ->> UI: Render New Data
UI -->> User: Display Updated Content
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range and nitpick comments (1)
examples/sites/demos/pc/app/tree/basic-usage.vue (1)
Line range hint
3-35:
Remove placeholder text and unnecessary commented-out code.The placeholder text "123" and the commented-out code should be removed if not necessary.
- 123 - <!-- <div class="option-row"> - <span> 连接线：</span> - <tiny-radio-group v-model="showLine"> - <tiny-radio label="show">显示</tiny-radio> - <tiny-radio label="hide">不显示</tiny-radio> - </tiny-radio-group> - </div> - <div class="option-row"> - <span> 组件大小：</span> - <tiny-radio-group v-model="size"> - <tiny-radio label="small">迷你尺寸</tiny-radio> - <tiny-radio label="medium">正常尺寸</tiny-radio> - </tiny-radio-group> - </div> - <div class="option-row"> - <span> 层级水平缩进：</span> - <tiny-radio-group v-model="indent"> - <tiny-radio :label="18">18px</tiny-radio> - <tiny-radio :label="28">28px</tiny-radio> - <tiny-radio :label="38">38px</tiny-radio> - </tiny-radio-group> - </div> - - <tiny-tree - :data="data" - :size="size" - :indent="indent" - :show-line="showLine === 'show'" - default-expand-all - @node-click="nodeClick" - ></tiny-tree> -->
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (10)
- examples/sites/demos/pc/app/tree/basic-usage.vue (2 hunks)
- internals/cli/src/commands/build/build-ui-solid.ts (1 hunks)
- packages/modules.json (2 hunks)
- packages/renderless/src/virtual-scroll/index.ts (1 hunks)
- packages/renderless/src/virtual-scroll/vue.ts (1 hunks)
- packages/vue/package.json (1 hunks)
- packages/vue/src/virtual-scroll/index.ts (1 hunks)
- packages/vue/src/virtual-scroll/package.json (1 hunks)
- packages/vue/src/virtual-scroll/src/index.ts (1 hunks)
- packages/vue/src/virtual-scroll/src/pc.vue (1 hunks)
Files skipped from review due to trivial changes (2)
- packages/modules.json
- packages/vue/package.json
Additional comments not posted (20)
packages/vue/src/virtual-scroll/src/pc.vue (4)
1-3: Placeholder template detected.The template section contains only a placeholder div. Ensure to replace this with the actual template for the virtual scroll component.
5-6: Ensure correct import paths.Verify that
@opentiny/vue-renderless/virtual-scroll/vueis the correct path for the imports.
11-11: Review prop spread and addition.The props spread operator and addition of
modelValueshould be verified to ensure no conflicts or issues.
13-14: LGTM!The setup function correctly initializes the component with the provided props and context.
packages/vue/src/virtual-scroll/package.json (3)
1-3: Ensure correct package name and version.Verify that the package name
@opentiny/vue-virtual-scrolland version3.17.0are correct and follow the project's versioning guidelines.
10-11: Review build scripts.Ensure that the build scripts are correct and follow the project's build process. The postversion script is commented out, verify if this is intentional.
13-20: Dependency check.Verify that the dependencies and devDependencies are correct and necessary for the package.
packages/vue/src/virtual-scroll/index.ts (4)
2-2: Ensure correct import path.Verify that the import path for the less file is correct or if it needs to be uncommented.
5-8: LGTM!The model definition for the VirtualScroll component is correct.
10-13: LGTM!The installation method for the VirtualScroll component is correctly defined.
15-21: LGTM!The version handling and runtime installation check for the VirtualScroll component are correctly defined.
packages/vue/src/virtual-scroll/src/index.ts (4)
3-3: Ensure correct import paths.Verify that the import paths for the less files are correct or if they need to be uncommented.
7-9: LGTM!The template function correctly selects the appropriate template based on the mode.
11-20: LGTM!The component definition correctly handles props, including the spread operator and default values.
21-23: LGTM!The setup function correctly initializes the component with the provided props and context.
packages/renderless/src/virtual-scroll/index.ts (3)
54-56: LGTM!The function is simple and correctly toggles the expand state.
58-61: LGTM!The function correctly computes the text field based on props and service.
63-66: LGTM!The function correctly computes the value field based on props and service.
examples/sites/demos/pc/app/tree/basic-usage.vue (1)
40-47: LGTM!The script section correctly imports and registers the
TinyVirtualScrollcomponent.internals/cli/src/commands/build/build-ui-solid.ts (1)
82-82: Verify the necessity of commenting out thevueTemplate2jsxplugin.The
vueTemplate2jsxplugin is commented out, which may affect the Vue-to-JSX transformation during the build process. Ensure that this change is intentional and does not introduce any issues.
|
|
||
| }) | ||
|
|
||
| // watch(() => props.modelValue, api.initUser, { immediate: true }) |
There was a problem hiding this comment.
Uncomment the watch statement or remove it if not needed.
The commented-out watch statement indicates potential incomplete functionality. Ensure it is necessary and uncomment it, or remove it if not needed.
- // watch(() => props.modelValue, api.initUser, { immediate: true })
+ watch(() => props.modelValue, api.initUser, { immediate: true })Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // watch(() => props.modelValue, api.initUser, { immediate: true }) | |
| watch(() => props.modelValue, api.initUser, { immediate: true }) |
| const state = reactive({ | ||
|
|
||
| }) |
There was a problem hiding this comment.
Incomplete state initialization.
The state object is initialized as an empty object. Ensure that all necessary state properties are defined.
- const state = reactive({
-
- })
+ const state = reactive({
+ user: null,
+ options: []
+ })Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const state = reactive({ | |
| }) | |
| const state = reactive({ | |
| user: null, | |
| options: [] | |
| }) |
| export const initUser = | ||
| ({ api, props, state }) => | ||
| (value) => { | ||
| if (!value) { | ||
| state.user = value | ||
| return | ||
| } | ||
|
|
||
| api.getUsers(value).then((info) => { | ||
| // 按value排序 | ||
| info.sort((a, b) => { | ||
| return value.indexOf(a[state.valueField] + '') > value.indexOf(b[state.valueField] + '') ? 1 : -1 | ||
| }) | ||
|
|
||
| const list = info.map((user) => { | ||
| return user[state.valueField] | ||
| }) | ||
|
|
||
| state.options = info | ||
| state.user = props.multiple ? list : list[0] | ||
| props.cache && api.cacheUser(info) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Ensure state.user is properly initialized.
The function sets state.user to the provided value or fetches user information. Ensure that the state.user property is properly initialized in the state object.
- ({ api, props, state }) =>
+ ({ api, props, state }) => {
+ state.user = null;
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const initUser = | |
| ({ api, props, state }) => | |
| (value) => { | |
| if (!value) { | |
| state.user = value | |
| return | |
| } | |
| api.getUsers(value).then((info) => { | |
| // 按value排序 | |
| info.sort((a, b) => { | |
| return value.indexOf(a[state.valueField] + '') > value.indexOf(b[state.valueField] + '') ? 1 : -1 | |
| }) | |
| const list = info.map((user) => { | |
| return user[state.valueField] | |
| }) | |
| state.options = info | |
| state.user = props.multiple ? list : list[0] | |
| props.cache && api.cacheUser(info) | |
| }) | |
| } | |
| export const initUser = | |
| ({ api, props, state }) => { | |
| state.user = null; | |
| return (value) => { | |
| if (!value) { | |
| state.user = value; | |
| return; | |
| } | |
| api.getUsers(value).then((info) => { | |
| // 按value排序 | |
| info.sort((a, b) => { | |
| return value.indexOf(a[state.valueField] + '') > value.indexOf(b[state.valueField] + '') ? 1 : -1; | |
| }); | |
| const list = info.map((user) => { | |
| return user[state.valueField]; | |
| }); | |
| state.options = info; | |
| state.user = props.multiple ? list : list[0]; | |
| props.cache && api.cacheUser(info); | |
| }); | |
| }; | |
| }; |
| export const showCard = | ||
| ({ api, service, state }) => | ||
| (e, user) => { | ||
| e.preventDefault() | ||
| state.spinner = true | ||
| state.expand = false | ||
|
|
||
| service.fetchUser(user.employeeNumber).then((data) => { | ||
| state.data = data | ||
| state.spinner = false | ||
| }) | ||
|
|
||
| api.getUserImageUrl(user.employeeNumber).then((url) => { | ||
| state.imgUrl = url | ||
| }) | ||
| } |
There was a problem hiding this comment.
Add error handling for asynchronous operations.
The function fetches user data and image URL without error handling. Ensure proper error handling for the asynchronous operations.
- service.fetchUser(user.employeeNumber).then((data) => {
- state.data = data
- state.spinner = false
- })
+ service.fetchUser(user.employeeNumber)
+ .then((data) => {
+ state.data = data
+ state.spinner = false
+ })
+ .catch((error) => {
+ console.error('Failed to fetch user data:', error)
+ state.spinner = false
+ })
- api.getUserImageUrl(user.employeeNumber).then((url) => {
- state.imgUrl = url
- })
+ api.getUserImageUrl(user.employeeNumber)
+ .then((url) => {
+ state.imgUrl = url
+ })
+ .catch((error) => {
+ console.error('Failed to fetch user image URL:', error)
+ })Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const showCard = | |
| ({ api, service, state }) => | |
| (e, user) => { | |
| e.preventDefault() | |
| state.spinner = true | |
| state.expand = false | |
| service.fetchUser(user.employeeNumber).then((data) => { | |
| state.data = data | |
| state.spinner = false | |
| }) | |
| api.getUserImageUrl(user.employeeNumber).then((url) => { | |
| state.imgUrl = url | |
| }) | |
| } | |
| export const showCard = | |
| ({ api, service, state }) => | |
| (e, user) => { | |
| e.preventDefault() | |
| state.spinner = true | |
| state.expand = false | |
| service.fetchUser(user.employeeNumber) | |
| .then((data) => { | |
| state.data = data | |
| state.spinner = false | |
| }) | |
| .catch((error) => { | |
| console.error('Failed to fetch user data:', error) | |
| state.spinner = false | |
| }) | |
| api.getUserImageUrl(user.employeeNumber) | |
| .then((url) => { | |
| state.imgUrl = url | |
| }) | |
| .catch((error) => { | |
| console.error('Failed to fetch user image URL:', error) | |
| }) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- .vscode/tasks.json (1 hunks)
- examples/sites/demos/pc/app/tree/basic-usage.vue (1 hunks)
- packages/renderless/src/virtual-scroll/index.ts (1 hunks)
- packages/renderless/src/virtual-scroll/vue.ts (1 hunks)
- packages/theme/src/virtual-scroll/index.less (1 hunks)
- packages/theme/src/virtual-scroll/vue.less (1 hunks)
- packages/vue/src/virtual-scroll/index.ts (1 hunks)
- packages/vue/src/virtual-scroll/src/index.ts (1 hunks)
- packages/vue/src/virtual-scroll/src/pc.vue (1 hunks)
Files skipped from review due to trivial changes (4)
- .vscode/tasks.json
- examples/sites/demos/pc/app/tree/basic-usage.vue
- packages/theme/src/virtual-scroll/index.less
- packages/theme/src/virtual-scroll/vue.less
Additional comments not posted (22)
packages/vue/src/virtual-scroll/index.ts (6)
1-4: LGTM!The import statements are necessary and correct.
6-9: LGTM!The model definition is standard and correct.
12-14: LGTM!The installation logic for the component is correct and follows common patterns.
16-16: LGTM!Assigning the version to the component is a good practice.
19-23: LGTM!The runtime installation logic ensures the component is available globally if Vue is detected. This is a good practice.
25-25: LGTM!The default export statement is correct.
packages/vue/src/virtual-scroll/src/index.ts (3)
1-4: LGTM!The import statements are necessary and correct. The commented-out mobile template import indicates potential future support for mobile.
6-10: LGTM!The constants object and template selection function are correctly defined.
12-26: LGTM!The component definition follows best practices and correctly initializes the component with props, context, and template.
packages/vue/src/virtual-scroll/src/pc.vue (3)
1-17: LGTM!The template section is well-structured and follows best practices for Vue components.
19-23: LGTM!The script section imports are necessary and correct.
25-34: LGTM!The component definition follows best practices and correctly initializes the component with props, context, renderless functions, and API.
packages/renderless/src/virtual-scroll/vue.ts (5)
1-11: Header looks good.The copyright and license information is standard.
12-16: Import statements and API definition look good.The imports and the
apiobject are defined correctly.
23-25: Incomplete state initialization.The state object is initialized as an empty object. Ensure that all necessary state properties are defined.
- const state = reactive({ - - }) + const state = reactive({ + user: null, + options: [] + })
32-32: Uncomment the watch statement or remove it if not needed.The commented-out watch statement indicates potential incomplete functionality. Ensure it is necessary and uncomment it, or remove it if not needed.
- // watch(() => props.modelValue, api.initUser, { immediate: true }) + watch(() => props.modelValue, api.initUser, { immediate: true })
33-35: Return statement looks good.The return statement correctly returns the
apiobject.packages/renderless/src/virtual-scroll/index.ts (5)
13-35: Ensurestate.useris properly initialized.The function sets
state.userto the provided value or fetches user information. Ensure that thestate.userproperty is properly initialized in the state object.- ({ api, props, state }) => + ({ api, props, state }) => { + state.user = null; + }
37-52: Add error handling for asynchronous operations.The function fetches user data and image URL without error handling. Ensure proper error handling for the asynchronous operations.
- service.fetchUser(user.employeeNumber).then((data) => { - state.data = data - state.spinner = false - }) + service.fetchUser(user.employeeNumber) + .then((data) => { + state.data = data + state.spinner = false + }) + .catch((error) => { + console.error('Failed to fetch user data:', error) + state.spinner = false + }) - api.getUserImageUrl(user.employeeNumber).then((url) => { - state.imgUrl = url - }) + api.getUserImageUrl(user.employeeNumber) + .then((url) => { + state.imgUrl = url + }) + .catch((error) => { + console.error('Failed to fetch user image URL:', error) + })
54-56: Function looks good.The function correctly toggles the
expandstate property.
58-61: Function looks good.The function correctly computes the text field based on the
propsandserviceobjects.
63-66: Function looks good.The function correctly computes the value field based on the
propsandserviceobjects.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/modules.json (1 hunks)
Additional comments not posted (2)
packages/modules.json (2)
3160-3167: Add documentation for the new component.The new component
VirtualScrollis added, but documentation for its usage is pending. Ensure to add relevant documentation to guide users on how to integrate and use this component.
3168-3172: Add documentation for the new template.The new template
VirtualScrollPcis added, but documentation for its usage is pending. Ensure to add relevant documentation to guide users on how to integrate and use this template.
|
[e2e-test-warn] The title of the Pull request should look like "fix(vue-renderless): [action-menu, alert] fix xxx bug". Please make sure you've read our contributing guide |
@necoxrr Virtual scrolling is a new component. Please refer to this document first, go through the component process, and ensure that after executing the [Steps to add new components](https://github.com/opentiny/tiny-vue/wiki/%E5%A2%9E%E5%8A%A0%E6%96%B0%E7%BB%84%E4 %BB%B6%E7%9A%84%E6%AD%A5%E9%AA%A4) |
| * | ||
| */ | ||
| // 具体操作方法和工具函数 | ||
| export const initUser = |
There was a problem hiding this comment.
@necoxrr 这个函数跟虚拟滚动似乎无关，包括以下的 showCard 函数
…-vue into feat/virtual_scroll
7612e75
into
opentiny:ospp-2024/001-tree-virtual-scroll
| */ | ||
| // start, end | ||
| // 二分法查找 | ||
| const binarySearch = (list, value) => { |
There was a problem hiding this comment.
类似这种比较通用的方法建议放到 utils 里 比如可以创建一个 utils 文件 与 组件功能无关的都有可以放进去
| */ | ||
| // start, end | ||
| // 二分法查找 | ||
| const binarySearch = (list, value) => { |
There was a problem hiding this comment.
建议这些方法都带上类型，因为涉及组件属性传递等方面，也方便后续维护是更通俗易懂
| } | ||
|
|
||
| // 计算数据总高度 | ||
| export const calculateTotalSize = |
There was a problem hiding this comment.
这段代码可以优化成
export const calculateTotalSize = ({ props, state }) => () => {
if (props.itemSize) {
return props.itemSize * state.data.length;
}
const lastPosition = state.positions?.[state.positions.length - 1];
return lastPosition ? lastPosition.bottom : 0;
};| await nextTick() | ||
| const size = virtualScroll.value.getBoundingClientRect()[props.direction === 'vertical' ? 'height' : 'width'] | ||
| const viewNum = Math.floor(size / (props.itemSize ?? props.estimatedItemSize)) | ||
| // console.log(virtualScroll.value.getBoundingClientRect().height, viewNum, state) |
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
New Features
Updates
VirtualScrollandVirtualScrollPcin modules to improve desktop interface functionality.@opentiny/vue-virtual-scroll.Enhancements