-
Notifications
You must be signed in to change notification settings - Fork 277
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
docs(grid): [grid] add updateFooter demo #2753
Conversation
WalkthroughThe pull request introduces enhancements to the grid components across multiple files, focusing on mobile-first support and footer calculation improvements. The changes primarily involve updating mode properties to support both 'pc' and 'mobile-first' configurations, modifying footer method calculations, and adding new demo components for grid footer functionality. These modifications aim to improve the grid component's flexibility and responsiveness across different device types. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Walkthrough此PR为 Changes
|
if (column.property === 'employees') { | ||
return data.map((item) => item[column.property]).reduce((acc, item) => acc + item) | ||
if (columnIndex === 2) { | ||
return data.map((item) => item.employees).reduce((acc, item) => acc + item) |
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.
使用columnIndex
代替column.property
来判断列是否为employees
,确保逻辑的一致性和简洁性。
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.
Actionable comments posted: 3
🧹 Nitpick comments (3)
examples/sites/demos/pc/app/grid/footer/update-footer.spec.ts (1)
3-11
: Add more test scenarios and assertions.While the test covers basic functionality, consider adding:
- Test for empty data state
- Test for multiple cell updates
- Verification of other cells remaining unchanged
- Error scenarios (e.g., invalid input)
test('表尾合并行或列', async ({ page }) => { page.on('pageerror', (exception) => expect(exception).toBeNull()) const demo = page.locator('#update-footer') await page.goto('grid-footer#update-footer') + // Test initial state await expect(demo.getByRole('cell', { name: '5310' })).toBeVisible() + + // Test empty data state + await demo.getByRole('button', { name: 'Clear Data' }).click() + await expect(demo.getByRole('cell', { name: '0' })).toBeVisible() + + // Test single update await demo.getByRole('cell', { name: '1300' }).click() await demo.locator('.tiny-numeric__increase').click() await expect(demo.getByRole('cell', { name: '5311' })).toBeVisible() + + // Test multiple updates + await demo.getByRole('cell', { name: '800' }).click() + await demo.locator('.tiny-numeric__increase').click() + await expect(demo.getByRole('cell', { name: '5312' })).toBeVisible() + + // Verify other cells unchanged + await expect(demo.getByRole('cell', { name: 'GFD 科技 YX 公司' })).toBeVisible() })examples/sites/demos/pc/app/grid/footer/update-footer-composition-api.vue (2)
85-103
: Optimize footerMethod implementation.The current implementation has several areas for improvement:
- Memoize calculations to avoid unnecessary recalculations
- Use more efficient array reduction
- Add input validation
+import { computed } from 'vue' + +const totalEmployees = computed(() => { + return tableData.value.reduce((sum, item) => sum + (item.employees || 0), 0) +}) + function footerMethod({ columns, data }) { + if (!Array.isArray(data) || !Array.isArray(columns)) { + console.warn('Invalid input to footerMethod') + return [['和值', '', '', 0]] + } + if (data.length === 0) { return [['和值', '', '', 0]] } return [ columns.map((column, columnIndex) => { if (columnIndex === 0) { return '和值' } if (column.property === 'employees') { - return data.map((item) => item[column.property]).reduce((acc, item) => acc + item) + return totalEmployees.value } return null }) ] }
3-14
: Add loading state handling.The grid should indicate when footer updates are in progress, especially for large datasets.
<tiny-grid ref="gridRef" :data="tableData" show-footer :footer-method="footerMethod" border - :edit-config="{}" + :edit-config="{ loading: updating }" >Add the following to the script section:
const updating = ref(false) async function handleChange() { updating.value = true try { await gridRef.value.updateFooter() } finally { updating.value = false } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
examples/sites/demos/apis/grid.js
(133 hunks)examples/sites/demos/pc/app/grid/footer/footer-row-or-column-span-composition-api.vue
(2 hunks)examples/sites/demos/pc/app/grid/footer/footer-row-or-column-span.vue
(2 hunks)examples/sites/demos/pc/app/grid/footer/update-footer-composition-api.vue
(1 hunks)examples/sites/demos/pc/app/grid/footer/update-footer.spec.ts
(1 hunks)examples/sites/demos/pc/app/grid/footer/update-footer.vue
(1 hunks)examples/sites/demos/pc/app/grid/webdoc/grid-footer.js
(1 hunks)
👮 Files not reviewed due to content moderation or server errors (3)
- examples/sites/demos/pc/app/grid/footer/footer-row-or-column-span.vue
- examples/sites/demos/pc/app/grid/footer/update-footer.vue
- examples/sites/demos/apis/grid.js
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: PR E2E Test (pnpm test:e2e3)
'demoId': 'update-footer', | ||
'name': { 'zh-CN': '手动更新表尾数据', 'en-US': '' }, | ||
'desc': { | ||
'zh-CN': | ||
'<p>默认情况下,只有当编辑失焦后才会更新表尾数据,通过 <code>updateFooter</code> 可以手动更新表尾数据。</p>\n', | ||
'en-US': '<p></p>' | ||
}, | ||
'codeFiles': ['footer/update-footer.vue'] |
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.
🛠️ Refactor suggestion
Add English translations for the new demo.
The demo configuration is missing English translations for both name and description. This affects accessibility for non-Chinese users.
{
'demoId': 'update-footer',
- 'name': { 'zh-CN': '手动更新表尾数据', 'en-US': '' },
+ 'name': { 'zh-CN': '手动更新表尾数据', 'en-US': 'Manually Update Footer Data' },
'desc': {
'zh-CN':
'<p>默认情况下,只有当编辑失焦后才会更新表尾数据,通过 <code>updateFooter</code> 可以手动更新表尾数据。</p>\n',
- 'en-US': '<p></p>'
+ 'en-US': '<p>By default, footer data is only updated when editing loses focus. You can manually update footer data using the <code>updateFooter</code> method.</p>\n'
},
'codeFiles': ['footer/update-footer.vue']
}
📝 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.
'demoId': 'update-footer', | |
'name': { 'zh-CN': '手动更新表尾数据', 'en-US': '' }, | |
'desc': { | |
'zh-CN': | |
'<p>默认情况下,只有当编辑失焦后才会更新表尾数据,通过 <code>updateFooter</code> 可以手动更新表尾数据。</p>\n', | |
'en-US': '<p></p>' | |
}, | |
'codeFiles': ['footer/update-footer.vue'] | |
'demoId': 'update-footer', | |
'name': { 'zh-CN': '手动更新表尾数据', 'en-US': 'Manually Update Footer Data' }, | |
'desc': { | |
'zh-CN': | |
'<p>默认情况下,只有当编辑失焦后才会更新表尾数据,通过 <code>updateFooter</code> 可以手动更新表尾数据。</p>\n', | |
'en-US': '<p>By default, footer data is only updated when editing loses focus. You can manually update footer data using the <code>updateFooter</code> method.</p>\n' | |
}, | |
'codeFiles': ['footer/update-footer.vue'] |
if (columnIndex === 2) { | ||
return data.map((item) => item.employees).reduce((acc, item) => acc + item) |
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.
Avoid hardcoding column indices.
Using hardcoded column indices (columnIndex === 2
) makes the code brittle. If columns are reordered or new columns are added, this logic will break.
- if (columnIndex === 2) {
- return data.map((item) => item.employees).reduce((acc, item) => acc + item)
+ if (column.property === 'employees') {
+ return data.map((item) => item[column.property]).reduce((acc, item) => acc + item)
}
Committable suggestion skipped: line range outside the PR's diff.
function handleChange() { | ||
gridRef.value.updateFooter() | ||
} |
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.
🛠️ Refactor suggestion
Add error handling to handleChange function.
The current implementation doesn't handle potential errors if the grid reference is undefined or if updateFooter fails.
function handleChange() {
+ try {
+ if (!gridRef.value) {
+ console.error('Grid reference is not available')
+ return
+ }
gridRef.value.updateFooter()
+ } catch (error) {
+ console.error('Failed to update footer:', 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.
function handleChange() { | |
gridRef.value.updateFooter() | |
} | |
function handleChange() { | |
try { | |
if (!gridRef.value) { | |
console.error('Grid reference is not available') | |
return | |
} | |
gridRef.value.updateFooter() | |
} catch (error) { | |
console.error('Failed to update footer:', error) | |
} | |
} |
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/vue/src/grid/src/grid/grid.ts (1)
524-525
: LGTM! Consider adding a fallback mechanism.The addition of the
IntersectionObserver
availability check is a good defensive programming practice that prevents runtime errors in unsupported environments.Consider implementing a fallback mechanism for browsers that don't support
IntersectionObserver
. For example:addIntersectionObserver() { if ((this.intersectionOption && this.intersectionOption.disabled) || typeof IntersectionObserver === 'undefined') - return + return this.handleVisibilityChange(true, { isIntersecting: true }) this.intersectionObserver = new IntersectionObserver((entries) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/vue/src/grid/src/grid/grid.ts
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: PR E2E Test (pnpm test:e2e3)
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/vue/src/grid/src/adapter/src/renderer.ts (1)
113-128
: Consider enhancing the empty option handling.The selection tracking and default empty option logic is good, but could be improved further.
Consider these enhancements:
- Make the empty option's text configurable through props
- Move the
display:none
style to CSS classes- optionsList.unshift(h('option', { style: 'display:none', selected: true }, '')) + const emptyOptionText = renderOpts.emptyOptionText || '' + optionsList.unshift(h('option', { + class: 'tiny-grid-select-empty-option', + selected: true + }, emptyOptionText))Add to your CSS:
.tiny-grid-select-empty-option { display: none; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/vue/src/grid/src/adapter/src/renderer.ts
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: PR E2E Test (pnpm test:e2e3)
🔇 Additional comments (2)
packages/vue/src/grid/src/adapter/src/renderer.ts (2)
103-108
: LGTM! Good improvement in variable declarations.Converting to
const
improves code safety by preventing accidental reassignment of these configuration values.
103-128
: Verify alignment with PR objectives.The changes improve select option rendering, but the PR objective mentions adding documentation and demo for the
updateFooter
API. Could you clarify how these changes relate to theupdateFooter
functionality?
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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/vue/src/grid/__tests__/grid.test.tsx (1)
93-93
: Remove redundantnextTick
call.The second
nextTick
call is redundant as the first one already ensures the component has been updated.- await nextTick()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/vue/src/grid/__tests__/grid.test.tsx
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: PR E2E Test (pnpm test:e2e3)
<GridColumn type="index" title="序号" width="100"></GridColumn> | ||
<GridColumn field="name" title="名称" width="100"> | ||
{{ | ||
default: (data) => ( | ||
<span class="pink-cell" style="color:pink"> | ||
{data.row.name} | ||
</span> | ||
) | ||
}}></GridColumn> | ||
<GridColumn field="city" titile="城市" width="100"></GridColumn> | ||
}} | ||
</GridColumn> | ||
<GridColumn field="city" title="城市" width="100"></GridColumn> |
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.
🛠️ Refactor suggestion
Enhance test coverage for slot functionality.
While the test verifies basic slot rendering, it should also verify:
- The correct rendering of the custom cell content
- The proper data binding in the slot scope
- The column width prop application
Consider expanding the test assertions:
await nextTick()
expect(wrapper.find('.pink-cell').exists()).toBeTruthy()
+ // Verify cell content
+ expect(wrapper.find('.pink-cell').text()).toBe('GFD科技YX公司')
+ // Verify column width
+ const nameColumn = wrapper.findAll('.tiny-grid-header__column')[1]
+ expect(nameColumn.attributes('style')).toContain('width: 100px')
Committable suggestion skipped: line range outside the PR's diff.
PR
新增表格updateFooter文档
updateFooter这个API一直存在,但是缺少文档说明与demo。
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
close #2078
Summary by CodeRabbit
New Features
Bug Fixes
Tests