-
Notifications
You must be signed in to change notification settings - Fork 331
fix(select): add some aria-* props for ai #3875
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
base: dev
Are you sure you want to change the base?
Conversation
WalkthroughAdds ARIA attributes to select and option Vue components and introduces a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (6)
🧰 Additional context used🧬 Code graph analysis (1)examples/sites/demos/pc/app/select/cache-usage.spec.ts (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/renderless/src/select/vue.ts(1 hunks)packages/vue/src/option/src/mobile-first.vue(1 hunks)packages/vue/src/option/src/pc.vue(1 hunks)packages/vue/src/select/src/mobile-first.vue(4 hunks)packages/vue/src/select/src/pc.vue(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: PR E2E Test (pnpm test:e2e3)
🔇 Additional comments (8)
packages/vue/src/option/src/mobile-first.vue (1)
24-26: ARIA attributes correctly implemented.The role and ARIA attributes are properly added for accessibility. The
aria-selectedattribute will be converted to a string by Vue's binding system.packages/vue/src/option/src/pc.vue (1)
20-22: ARIA attributes correctly implemented.The role and ARIA attributes are properly added, aligning with WCAG guidelines for option elements within a listbox.
packages/vue/src/select/src/pc.vue (2)
297-301: ARIA combobox pattern correctly implemented.The role and ARIA attributes properly establish the combobox pattern with correct ownership and control relationships to the listbox.
362-363: Listbox role and ID correctly linked.The listbox role and ID binding complete the ARIA ownership relationship, enabling proper screen reader navigation.
packages/vue/src/select/src/mobile-first.vue (4)
248-252: ARIA combobox pattern correctly implemented on filterable input.The ARIA attributes properly establish combobox semantics with ownership linkage to the listbox.
285-289: ARIA combobox pattern correctly implemented on main input.The ARIA attributes align with the combobox pattern. Note that both this input and the filterable input (line 215) reference the same
ariaListId, which is acceptable since their visibility is controlled by mutually exclusive conditions (filterableand!shape).
353-354: Listbox role and ID correctly linked.The listbox role and ID binding complete the ARIA relationship with the combobox inputs.
466-469: Code formatting updated.The class binding has been reformatted to consistently use the
m()helper function. No functional change.
| })), | ||
| ariaListId: 'tiny-select-' + crypto.randomUUID().slice(-8) |
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.
🧩 Analysis chain
🏁 Script executed:
# Check for browserslist and browser support configuration
fd -t f "browserslist|.browserslistrc" || echo "No browserslist found"
# Also check package.json for engine constraints and browser field
cat package.json | head -50Repository: opentiny/tiny-vue
Length of output: 1731
🏁 Script executed:
# Search for crypto.randomUUID usage in the codebase
rg "crypto\.randomUUID|crypto\.getRandomValues" -A 2 -B 2Repository: opentiny/tiny-vue
Length of output: 4224
🏁 Script executed:
# Check if there are any polyfills or UUID libraries already in use
rg "uuid|polyfill|fillRandom|getRandomValues" -l | head -20Repository: opentiny/tiny-vue
Length of output: 385
🏁 Script executed:
# Look for browserslist configuration in various locations
find . -maxdepth 2 -type f \( -name ".browserslistrc" -o -name "browserslist" \) -o -path "*/package.json" | head -10Repository: opentiny/tiny-vue
Length of output: 75
🏁 Script executed:
# Check if there's a browserslist config in root or package.json
cat package.json | grep -A 10 "browserslist"Repository: opentiny/tiny-vue
Length of output: 120
🏁 Script executed:
# Check the nanoid utility since it has IE polyfills
head -80 packages/utils/src/nanoid/index.tsRepository: opentiny/tiny-vue
Length of output: 1674
🏁 Script executed:
# Check how generateID is implemented in the codebase
cat packages/vue/src/query-builder/src/utils/generateID.tsRepository: opentiny/tiny-vue
Length of output: 2040
Replace direct crypto.randomUUID() with the project's existing generateID() utility.
The codebase uses crypto.randomUUID() without fallback, which is inconsistent with the established pattern. The project's generateID() utility in packages/vue/src/query-builder/src/utils/generateID.ts provides proper browser compatibility handling with fallbacks for environments where randomUUID() is unavailable.
- ariaListId: 'tiny-select-' + crypto.randomUUID().slice(-8)
+ ariaListId: 'tiny-select-' + generateID().slice(-8)Import generateID from @opentiny/utils at the top of the file.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/renderless/src/select/vue.ts around lines 299-300, replace the
direct use of crypto.randomUUID() for ariaListId with the project's generateID()
utility to ensure browser compatibility; add an import for generateID from
'@opentiny/utils' at the top of the file and use generateID().slice(-8) (or
otherwise adapt to generateID() return format) when constructing 'tiny-select-'
+ ... so it mirrors the existing pattern and fallback handling.
PR
为select 添加 aria-* 的一些属性,方便ai识别读取
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
Tests
✏️ Tip: You can customize this high-level summary in your review settings.