fix(popper-arrow): [tooltip, popover,dropdown] fix arrow's z-index, and remove popper-arrow class from popover,dropdown #2446
Conversation
… from popover,dropdown
WalkthroughThe changes primarily involve the restructuring and simplification of CSS styles related to popper elements, specifically the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (2)
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 (
|
|
[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 |
shenjunjian
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (16)
examples/sites/demos/pc/app/color-picker/predefine-composition-api.vue (1)
3-6: Consider adding aria-labels for better accessibility.While the component migration looks good, consider enhancing accessibility by adding descriptive aria-labels to the interactive elements.
- <tiny-color-picker v-model="color" :predefine="predefine" /> + <tiny-color-picker v-model="color" :predefine="predefine" aria-label="Color selection" /> - <tiny-button @click="addPredefineColor">Append predefine color</tiny-button> + <tiny-button @click="addPredefineColor" aria-label="Add new predefined color">Append predefine color</tiny-button> - <tiny-button @click="popPredefineColor">Pop predefine color</tiny-button> + <tiny-button @click="popPredefineColor" aria-label="Remove last predefined color">Pop predefine color</tiny-button>examples/sites/demos/pc/app/slider/max-min.spec.ts (1)
16-20: Consider adding comments to explain value expectations.The relationship between mouse movements and expected values isn't immediately obvious. Consider adding comments to explain why these specific values are expected.
await page.mouse.move(sliderWidth * -0.3 + x, y) const sliderTip = slider.locator('div').nth(2) + // Expect 30 when slider is moved 30% to the left await expect(sliderTip).toHaveText('30') await page.waitForTimeout(1000) await page.mouse.move(sliderWidth * 0.81 + x, y) const sliderTip1 = slider.locator('div').nth(2) + // Expect 62 when slider is moved 81% to the right await expect(sliderTip1).toHaveText('62')examples/sites/demos/pc/app/color-select-panel/predefine-composition-api.vue (1)
Line range hint
6-14: Consider z-index management improvements.The color panel is wrapped in a relatively positioned div, which is good for positioning. However, since this PR aims to fix popper arrow z-index issues, consider adding explicit z-index management to ensure proper stacking context:
- <div style="position: relative"> + <div style="position: relative; z-index: 0"> <tiny-color-select-panel v-model="color" :visible="visible" @confirm="onConfirm" @cancel="onCancel" :predefine="predefine" alpha /> </div>This ensures a new stacking context is created, which can help prevent z-index conflicts with other components.
examples/sites/demos/pc/app/select/nest-radio-grid-much-data.spec.ts (1)
Line range hint
29-34: Add comments explaining the scroll behavior test.The repeated scroll and count verification appears to be testing an important behavior. Consider adding a comment explaining the purpose of these repeated checks.
await expect(page.getByRole('row', { name: '华南区12 广东省 广州市' })).toBeHidden() + // Verify that scrolling maintains consistent row count and doesn't trigger unnecessary re-renders await row.nth(5).scrollIntoViewIfNeeded() await expect(row).toHaveCount(6) await row.nth(5).scrollIntoViewIfNeeded() await expect(row).toHaveCount(6) await row.nth(5).scrollIntoViewIfNeeded() await expect(row).toHaveCount(6)packages/theme/src/month-table/vars.less (1)
37-39: Fix indentation for consistency.The indentation of the new variables is inconsistent with the rest of the file. The continuation line should align with the start of the value.
- --tv-MonthTable-cell-border-radius-first: var(--tv-border-radius-round, 999px) 0 0 - var(--tv-border-radius-round, 999px); + --tv-MonthTable-cell-border-radius-first: var(--tv-border-radius-round, 999px) 0 0 var(--tv-border-radius-round, 999px);examples/sites/demos/pc/app/grid/editor/popeditor-in-grid-remote-search.vue (1)
Line range hint
127-146: Consider production-ready improvements for remote search.While this is a demo implementation, for production environments consider:
- Implementing proper error handling for the remote search
- Adding loading states during data fetching
- Implementing request debouncing/throttling for search
- Using environment variables for pagination limits instead of hardcoded values
examples/sites/demos/pc/app/grid/renderer/async-colunm-render.vue (1)
Line range hint
155-182: Consider enhancing the async implementation.While the current implementation works, there are several areas where it could be improved:
- Error handling for async operations
- Loading states for better UX
- More meaningful city codes in the demo data
Consider applying these improvements:
methods: { fetchMethod() { + if (this.loading) return Promise.reject(new Error('Request in progress')) + this.loading = true + const randomFN = () => { return [ { - text: '福州', - value: 'FZ' + text: '福州', + value: 'fuzhou' }, { - text: '深圳', - value: 'SZ' + text: '深圳', + value: 'shenzhen' }, // ... other cities ] } return new Promise((resolve, reject) => { - setTimeout(() => { - resolve(randomFN()) - }, 500) + try { + // Simulate API call + setTimeout(() => { + const data = randomFN() + resolve(data) + }, 500) + } catch (error) { + reject(error) + } finally { + this.loading = false + } }) } }Also, consider adding a loading indicator to the template:
<tiny-grid-column field="city" title="所在城市" :editor="{ component: TinySelect, - attrs: { textField: 'text', clearable: true } + attrs: { + textField: 'text', + clearable: true, + loading: loading + } }" > </tiny-grid-column>packages/theme/src/tag/vars.less (1)
Line range hint
27-69: Consider documenting the size progression rationale.The size progression follows a clear pattern:
- Font sizes: 12px → 14px → 16px → 20px
- Padding: 2px → 2px → 3px → 5px
Consider adding a comment block explaining this progression to help maintainers understand the design decisions.
Add this comment block before the mini size variables:
+ // Size progression for icon-only mode: + // - Font sizes increase by ~2-4px: 12px → 14px → 16px → 20px + // - Padding increases gradually: 2px → 2px → 3px → 5px +examples/sites/demos/pc/app/grid/large-data/grid-large-tree-data-composition-api.vue (1)
Line range hint
94-116: Consider optimizing duplicate column definitions.The grid contains multiple columns with identical configurations but different numbered suffixes (e.g., "需求来源", "需求来源2", "需求来源3"). This pattern could be simplified to improve maintainability and potentially performance.
Consider generating these columns programmatically:
- <tiny-grid-column field="requireSource" title="需求来源2"></tiny-grid-column> - <tiny-grid-column field="isRequireDev" title="是否需要开发2"></tiny-grid-column> - <!-- ... more duplicate columns ... --> + <template v-for="suffix in ['', '2', '3']" :key="suffix"> + <tiny-grid-column + :field="'requireSource'" + :title="`需求来源${suffix}`" + ></tiny-grid-column> + <tiny-grid-column + :field="'isRequireDev'" + :title="`是否需要开发${suffix}`" + ></tiny-grid-column> + <!-- ... other columns ... --> + </template>examples/sites/demos/pc/app/grid/large-data/grid-large-tree-data.vue (1)
Line range hint
1-120: Consider refactoring duplicate columns.The grid contains multiple sets of identical columns with suffixes '2' and '3' (e.g., "需求来源", "需求来源2", "需求来源3"). This duplication increases maintenance overhead and reduces code maintainability.
Consider:
- Using a dynamic column generation approach
- Evaluating if these duplicate columns serve a specific purpose, if not, consider removing them
Example refactor:
const baseColumns = [ { field: 'requireSource', title: '需求来源' }, { field: 'isRequireDev', title: '是否需要开发' }, // ... other base columns ]; // In template baseColumns.map((col, index) => ( <tiny-grid-column :key="index" :field="col.field" :title="index === 0 ? col.title : `${col.title}${index + 1}`" > <template #header="{ column }">{{ column.title }}</template> <template #default="{ row, column }">{{ row[column.property] }}</template> </tiny-grid-column> ))packages/theme/src/select/index.less (1)
Line range hint
348-386: LGTM! Consider documenting the padding values.The padding adjustments for disabled and display-only states look correct and maintain visual consistency. The differentiation between regular state (16px) and show-tag state (32px) is well-structured.
Consider adding a comment explaining the padding values or moving them to CSS variables for better maintainability:
.@{select-prefix-cls}__tags { - padding-right: 16px; + padding-right: var(--tv-Select-tags-padding-right, 16px); } .@{select-prefix-cls}__tags.is-show-tag { - padding-right: 32px; + padding-right: var(--tv-Select-tags-padding-right-with-tag, 32px); }packages/vue-icon-saas/index.ts (2)
Line range hint
1-518: Consider grouping related icons together.The file has a large number of icon imports that could benefit from better organization. Consider grouping related icons together (e.g., editor icons, navigation icons, status icons) and adding section comments to improve maintainability.
Example organization:
// Navigation Icons import IconArrowLeft from './src/arrow-left' import IconArrowRight from './src/arrow-right' import IconArrowUp from './src/arrow-up' import IconArrowDown from './src/arrow-down' // Editor Icons import IconEditorBold from './src/editor-bold' import IconEditorItalic from './src/editor-italic' import IconEditorUnderline from './src/editor-underline' // Status Icons import IconSuccess from './src/success' import IconError from './src/error' import IconWarning from './src/warning'
Line range hint
1-2446: Consider splitting the file for better maintainability.The file is quite large with over 2000 lines. Consider splitting it into smaller, more focused files based on icon categories. This would make the codebase more maintainable and easier to navigate.
Suggested structure:
- Create separate files for different icon categories
- Use a barrel file pattern for exports
- Example:
// navigation-icons.ts export { IconArrowLeft, IconArrowRight, /* ... */ } // editor-icons.ts export { IconEditorBold, IconEditorItalic, /* ... */ } // index.ts (barrel file) export * from './navigation-icons' export * from './editor-icons' // ... other exportspackages/vue/src/select/src/pc.vue (3)
159-159: Clarify the Boolean Prop Usage foronly-iconIn line 159, the
only-iconattribute is used without an explicit value. While Vue treats presence of the attribute astrue, it's clearer and more maintainable to bind the prop explicitly.Apply this diff to make the binding explicit:
- only-icon + :only-icon="true"
Line range hint
652-656: Define Prop Types and Default Values for New PropsThe new props
searchableandshowEmptyImagelack explicit type definitions and default values. Defining them enhances code readability and prevents unintended behavior if the props are not provided.Apply this diff to define their types and default values:
export default defineComponent({ //... - props: [ - //... - 'searchable', - 'showEmptyImage', - //... - ], + props: { + //... existing prop definitions + searchable: { + type: Boolean, + default: false + }, + showEmptyImage: { + type: Boolean, + default: false + }, + //... other prop definitions + },
Line range hint
362-373: Consider Debouncing the Search InputTo improve performance and user experience during rapid input, consider debouncing the search method to limit how often it triggers.
You can implement debouncing as follows:
+import { debounce } from 'lodash'; export default defineComponent({ //... setup(props, context) { //... + const handleQueryChangeDebounced = debounce((query) => { + api.handleQueryChange(query); + }, 300); + + return { + //... + handleQueryChangeDebounced, + //... + }; }, }); // Update the event listener in the template<tiny-input v-if="searchable" input-box-type="underline" v-model="state.query" :placeholder="t('ui.search.placeholder')" class="tiny-select-dropdown__search" - @update:modelValue="handleQueryChange(state.query)" + @input="handleQueryChangeDebounced" >
🛑 Comments failed to post (11)
examples/sites/demos/pc/app/tag/size-composition-api.vue (1)
17-19: 🛠️ Refactor suggestion
Consider moving icon initialization inside template
While the current implementation works, you could simplify the code by directly using the IconHeartempty component in the template without initializing it as a separate constant.
import { IconHeartempty } from '@opentiny/vue-icon' - -const TinyIconHeartempty = IconHeartempty()Then in template:
-<tiny-icon-heartempty /> +<icon-heartempty />This approach reduces code verbosity while maintaining the same functionality.
Committable suggestion was skipped due to low confidence.
examples/sites/demos/pc/app/tag/size.vue (1)
21-22:
⚠️ Potential issueFix the icon component registration.
The current registration immediately executes the icon component function. This is unnecessary and might cause issues.
Apply this fix:
components: { TinyTag, - TinyIconHeartempty: IconHeartempty() + TinyIconHeartempty: IconHeartempty }📝 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.TinyTag, TinyIconHeartempty: IconHeartemptyexamples/sites/demos/pc/app/select/nest-radio-grid-much-data.spec.ts (1)
26-26: 💡 Codebase verification
Inconsistent class names detected in test files
The verification reveals inconsistency in class naming across test files:
row__selectedis still used in:
examples/sites/demos/pc/app/select/nest-grid.spec.tsexamples/sites/demos/pc/app/select/map-field.spec.tsrow__radiois used in:
examples/sites/demos/pc/app/select/nest-radio-grid-much-data.spec.tsexamples/sites/demos/pc/app/select/nest-grid.spec.ts(uses both classes)examples/sites/demos/pc/app/select/nest-grid-remote.spec.ts🔗 Analysis chain
LGTM! Verify class name consistency across components.
The updated class assertion from
row__selectedtorow__radioaligns with the component changes.Let's verify the consistency of this class name change across the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining instances of the old class name and verify the new class name usage # Expected: No instances of row__selected in test files, consistent usage of row__radio # Check for any remaining instances of the old class name in test files echo "Checking for old class name usage..." rg "row__selected" -g "*.spec.ts" # Verify the new class name usage echo "Verifying new class name usage..." rg "row__radio" -g "*.spec.ts"Length of output: 883
examples/sites/demos/pc/app/select/nest-grid-remote.vue (1)
122-122:
⚠️ Potential issueRemove console.log statement from production code.
Console statements should not be committed to production code as they can impact performance and potentially expose sensitive data. If this was added for debugging purposes, please remove it before merging.
- console.log(filterData, value)📝 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.examples/sites/demos/pc/app/grid/validation/valid-config-composition-api.vue (1)
18-21: 🛠️ Refactor suggestion
Consider standardizing all editor components.
While the
namefield now usesTinyInput, other fields still use the string literal'input'. Consider standardizing all editor components to useTinyInputfor consistency:<tiny-grid-column field="name" title="名称" :editor="{ component: TinyInput }"></tiny-grid-column> - <tiny-grid-column field="area" title="区域" :editor="{ component: 'input' }"></tiny-grid-column> - <tiny-grid-column field="address" title="地址" :editor="{ component: 'input' }"></tiny-grid-column> - <tiny-grid-column field="introduction" title="公司简介" :editor="{ component: 'input' }"></tiny-grid-column> + <tiny-grid-column field="area" title="区域" :editor="{ component: TinyInput }"></tiny-grid-column> + <tiny-grid-column field="address" title="地址" :editor="{ component: TinyInput }"></tiny-grid-column> + <tiny-grid-column field="introduction" title="公司简介" :editor="{ component: TinyInput }"></tiny-grid-column>📝 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.<tiny-grid-column field="name" title="名称" :editor="{ component: TinyInput }"></tiny-grid-column> <tiny-grid-column field="area" title="区域" :editor="{ component: TinyInput }"></tiny-grid-column> <tiny-grid-column field="address" title="地址" :editor="{ component: TinyInput }"></tiny-grid-column> <tiny-grid-column field="introduction" title="公司简介" :editor="{ component: TinyInput }"></tiny-grid-column>examples/sites/demos/pc/app/grid/validation/valid-config.vue (1)
18-21: 🛠️ Refactor suggestion
Standardize editor component configuration across columns.
There's an inconsistency in how editor components are specified:
namecolumn uses component reference::editor="{ component: TinyInput }"- Other columns use string notation:
:editor="{ component: 'input' }"Consider standardizing this approach across all columns for better maintainability.
Apply this pattern to all columns:
- <tiny-grid-column field="name" title="名称" :editor="{ component: TinyInput }"></tiny-grid-column> - <tiny-grid-column field="area" title="区域" :editor="{ component: 'input' }"></tiny-grid-column> - <tiny-grid-column field="address" title="地址" :editor="{ component: 'input' }"></tiny-grid-column> - <tiny-grid-column field="introduction" title="公司简介" :editor="{ component: 'input' }"></tiny-grid-column> + <tiny-grid-column field="name" title="名称" :editor="{ component: TinyInput }"></tiny-grid-column> + <tiny-grid-column field="area" title="区域" :editor="{ component: TinyInput }"></tiny-grid-column> + <tiny-grid-column field="address" title="地址" :editor="{ component: TinyInput }"></tiny-grid-column> + <tiny-grid-column field="introduction" title="公司简介" :editor="{ component: TinyInput }"></tiny-grid-column>📝 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.<tiny-grid-column field="name" title="名称" :editor="{ component: TinyInput }"></tiny-grid-column> <tiny-grid-column field="area" title="区域" :editor="{ component: TinyInput }"></tiny-grid-column> <tiny-grid-column field="address" title="地址" :editor="{ component: TinyInput }"></tiny-grid-column> <tiny-grid-column field="introduction" title="公司简介" :editor="{ component: TinyInput }"></tiny-grid-column>packages/theme/src/base/reset.less (2)
194-224: 🛠️ Refactor suggestion
Consider improving arrow positioning consistency and animations.
The arrow positioning uses different offset values (-7px vs -3px) for vertical and horizontal placements, which might cause alignment inconsistencies.
Consider these improvements:
- Standardize positioning offsets:
&[x-placement^='right'] { margin-left: 12px; .popper__arrow { - left: -3px; + left: -7px; } } &[x-placement^='left'] { margin-right: 12px; .popper__arrow { - right: -3px; + right: -7px; } }
- Add smooth transitions for better UX:
.popper__arrow { transition: transform 0.2s ease-in-out; }
180-192: 🛠️ Refactor suggestion
⚠️ Potential issueReconsider the negative z-index approach for the popper arrow.
Setting
z-index: -1on the arrow could cause it to be hidden behind the popper's content. This might be the root cause of the visibility issues.Consider these alternatives:
.popper__arrow { position: absolute; display: block; width: 12px; height: 20px; background-color: #fff; transform: rotateZ(45deg); border-radius: 2px; - z-index: -1; + z-index: 0; }Also consider:
- Using CSS custom properties for the background color to support theming
- Making the dimensions configurable via CSS variables
Committable suggestion was skipped due to low confidence.
examples/sites/demos/pc/app/grid/validation/validation-scroll-to-col-composition-api.vue (1)
14-14: 💡 Codebase verification
Migration pattern is partially implemented
The search results reveal an inconsistency in the component migration pattern:
- All files correctly import
TinyInputfrom '@opentiny/vue'- Each file has one column using
TinyInput(for the "名称" field)- However, all files still have another column using lowercase
'input'(for the "区域" field)This suggests that the component migration is incomplete. The following locations need attention:
- examples/sites/demos/pc/app/grid/validation/valid-config.vue
- examples/sites/demos/pc/app/grid/validation/validation-scroll-to-col.vue
- examples/sites/demos/pc/app/grid/validation/validation-scroll-to-col-composition-api.vue
- examples/sites/demos/pc/app/grid/validation/valid-config-composition-api.vue
🔗 Analysis chain
LGTM! Component migration looks good.
The change from
InputtoTinyInputis consistent with the ongoing component migration pattern and is properly imported in the script section.Let's verify if this migration pattern is consistent across other grid validation examples:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining Input components in grid validation files rg -l "component: Input" "examples/sites/demos/pc/app/grid/validation/"Length of output: 71
Script:
#!/bin/bash # Let's try a broader search pattern to find any Input or TinyInput usage in grid validation files rg -A 1 "component: (Input|TinyInput)" "examples/sites/demos/pc/app/grid/validation/" # Also search for import statements to verify the component migration rg "(import.*Input)|(Input.*from)" "examples/sites/demos/pc/app/grid/validation/"Length of output: 2290
packages/theme/src/tag/index.less (2)
53-61: 🛠️ Refactor suggestion
Improve code consistency and maintainability.
A few suggestions for improvement:
- The double repetition of
.@{tag-prefix-cls}--only-iconcreates the same maintainability concerns.- The comment is in Chinese while other comments are in English.
- &.@{tag-prefix-cls}--only-icon.@{tag-prefix-cls}--only-icon { + &[class*="--only-icon"] { padding: e(@icon-py); svg { font-size: e(@icon-fs); - margin-right: 0; // 此时不需要右边距了 + margin-right: 0; // Remove right margin for icon-only tags } }📝 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.&[class*="--only-icon"] { padding: e(@icon-py); svg { font-size: e(@icon-fs); margin-right: 0; // Remove right margin for icon-only tags } }
32-39: 🛠️ Refactor suggestion
Consider refactoring the specificity hack.
The triple repetition of
.@{tag-prefix-cls}--only-iconis a specificity hack that could make the styles harder to maintain. Consider using a more maintainable approach:- &.@{tag-prefix-cls}--only-icon.@{tag-prefix-cls}--only-icon.@{tag-prefix-cls}--only-icon { + &[class*="--only-icon"] { padding: var(--tv-Tag-only-icon-padding); svg { font-size: var(--tv-Tag-only-icon-font-size); - margin-right: 0; // 此时不需要右边距了 + margin-right: 0; } }Alternatively, consider using CSS custom properties to control these styles, which would provide better maintainability and flexibility.
Committable suggestion was skipped due to low confidence.
…nd remove popper-arrow class from popover,dropdown (#2446) * fix(popper-arrow): fix arrow's z-index, and remove popper-arrow class from popover,dropdown * fix(popover): remove extra cssvars * fix(user-contact): add margin:8px
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
Release Notes
New Features
Bug Fixes
Chores
Style
These updates improve the overall user experience by simplifying component designs and ensuring consistent styling.