feat(layer): enhance MDC tabs with icons, type safety, and accessibility#6
feat(layer): enhance MDC tabs with icons, type safety, and accessibility#6
Conversation
- Add reference submodules: nuxt/content, nuxt-content/mdc, nuxt/ui
- Implement Tabs and TabsItem content components for MDC syntax
- Add MDC component mapping in nuxt.config.ts
- Update UPageCard to apply padding on default slot
- Update index.md to use new ::tabs-item MDC syntax
- Update ref-analysis.md with Nuxt Content, MDC, and Nuxt UI analysis
- Add MDC component mapping documentation
BREAKING CHANGE: Tabs now use ::tabs-item{label="..."} syntax instead of
::tabs-list/::tabs-trigger/::tabs-content
- Fix index bug in slot transformation for fragment handling - Add icon prop support with aria-hidden for accessibility - Replace any types with VNode for type safety - Update documentation for new ::tabs-item API - Add aria-hidden to UPageCard decorative icons
Summary of ChangesHello @amondnet, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly upgrades the MDC tabs component by introducing icon support, simplifying its API for developers, and boosting both type safety and accessibility. It also resolves a crucial bug related to slot transformation, ensuring robust handling of tab content. These changes collectively aim to provide a more user-friendly, developer-efficient, and accessible tab system. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request is a great enhancement to the tabs component, introducing icon support, a simpler API, and accessibility improvements. The refactoring from multiple components to a single Tabs component that processes TabsItem children is a solid approach for the MDC context. My review includes a couple of suggestions for the Tabs.vue component to improve its efficiency and robustness by refactoring the slot-handling logic.
| const rerenderCount = ref(1) | ||
|
|
||
| interface TabItem { | ||
| value: string | ||
| label: string | ||
| icon?: string | ||
| component: VNode | ||
| } | ||
|
|
||
| const items = computed<TabItem[]>(() => { | ||
| // eslint-disable-next-line @typescript-eslint/no-unused-expressions | ||
| rerenderCount.value | ||
|
|
||
| let counter = 0 | ||
| function transformSlot(slot: VNode): TabItem | null { | ||
| if (typeof slot.type === 'symbol') { | ||
| return null // Fragments are handled by flatMap in parent call | ||
| } | ||
|
|
||
| const slotProps = slot.props as { label?: string, icon?: string } | null | ||
| if (!slotProps?.label) { | ||
| return null | ||
| } | ||
|
|
||
| return { | ||
| value: String(counter++), | ||
| label: slotProps.label, | ||
| icon: slotProps.icon, | ||
| component: slot, | ||
| } | ||
| } | ||
|
|
||
| function flattenSlots(slot: VNode): VNode[] { | ||
| if (typeof slot.type === 'symbol') { | ||
| const children = slot.children as VNode[] | null | ||
| return children?.flatMap(flattenSlots) ?? [] | ||
| } | ||
| return [slot] | ||
| } | ||
|
|
||
| return slots.default?.()?.flatMap(flattenSlots).map(transformSlot).filter((item): item is TabItem => item !== null) || [] | ||
| }) | ||
|
|
||
| onMounted(() => { | ||
| if (props.sync) { | ||
| const syncKey = `tabs-${props.sync}` | ||
| const stored = localStorage.getItem(syncKey) | ||
|
|
||
| if (stored && items.value.some(item => item.value === stored)) { | ||
| model.value = stored | ||
| } | ||
|
|
||
| watch(model, (value) => { | ||
| if (value) { | ||
| localStorage.setItem(syncKey, value) | ||
| } | ||
| }) | ||
| } | ||
| }) | ||
|
|
||
| onBeforeUpdate(() => rerenderCount.value++) |
There was a problem hiding this comment.
The use of rerenderCount (line 36), which is incremented in onBeforeUpdate (line 96), to force re-computation of the items computed property is a common but potentially inefficient pattern. It causes the items to be recalculated on every component update, not just when slots change.
A cleaner and more performant approach would be to calculate the items within onBeforeUpdate and store them in a ref. This also allows helper functions to be defined only once.
Suggested Refactoring:
- Replace
rerenderCountand theitemscomputed property with aref:const items = ref<TabItem[]>([]
- Move the logic for calculating items into a function, e.g.,
calculateItems.function calculateItems() { // ... logic from computed property ... items.value = ... }
- Call this function in
onMountedandonBeforeUpdate.onMounted(() => { calculateItems() // ... rest of onMounted }) onBeforeUpdate(() => { calculateItems() })
This avoids the rerenderCount variable and makes the component's reactivity more explicit and efficient.
| function flattenSlots(slot: VNode): VNode[] { | ||
| if (typeof slot.type === 'symbol') { | ||
| const children = slot.children as VNode[] | null | ||
| return children?.flatMap(flattenSlots) ?? [] | ||
| } | ||
| return [slot] | ||
| } |
There was a problem hiding this comment.
The flattenSlots function uses typeof slot.type === 'symbol' to identify fragments. While this works for Fragment, it's a broad check that could also match other special VNode types like Comment or Text nodes. This might lead to unexpected behavior if they are passed as direct children. It's safer and more explicit to check specifically for Fragment.
You'll also need to import Fragment from vue:
import { Fragment, ... } from 'vue'
function flattenSlots(slot: VNode): VNode[] {
if (slot.type === Fragment) {
const children = slot.children as VNode[] | null
return children?.flatMap(flattenSlots) ?? []
}
return [slot]
}
Summary
This PR enhances the MDC tabs component with icon support, improved type safety, and accessibility improvements. The implementation fixes a critical index bug in slot transformation, eliminates type safety violations, and improves the API documentation.
Key Changes
Bug Fixes
Features
iconprop with proper accessibility attributes::tabs-item{label="..."}syntaxCode Quality
anytypes with properVNodetypes throughout componentsaria-hidden="true"to decorative icons in Tabs and UPageCard componentsFiles Changed
packages/layer/app/components/content/Tabs.vue- Enhanced with VNode types, icon support, fragment handlingpackages/layer/app/components/content/TabsItem.vue- Fixed VNode types in slot definitionspackages/layer/app/components/content/UPageCard.vue- Added aria-hidden for decorative iconsapps/docs/content/docs/2.components/6.tabs.md- Updated documentation with new APIapps/docs/content/index.md- Added icons to tabs examplespackages/layer/app/components/app/AppFooter.vue- Minor text updateTesting
Checklist
anytypes)