fix(core/dropdown-button): accessibility consistency in dropdown-button-icon - #2654
fix(core/dropdown-button): accessibility consistency in dropdown-button-icon#2654GayatriK2002 wants to merge 4 commits into
Conversation
✅ Deploy Preview for ix-storybook canceled.
|
📝 WalkthroughWalkthroughChangesDropdown preview updates
Vue prop handling
TypeScript target updates
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces layout adjustments to dropdown buttons, updates TypeScript configurations to target ES2020, and changes the EMPTY_PROP sentinel from a Symbol to an object literal in Vue utilities, accompanied by a patch for @stencil/vue-output-target. Feedback on these changes advises against using patch-package in a pnpm workspace to avoid global store corruption, recommending pnpm's native patching mechanism instead. Additionally, it is recommended to freeze the newly defined EMPTY_PROP object using Object.freeze() to prevent accidental mutations.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| "prepare": "pnpm disable-telemetry && patch-package --patch-dir patches --patch-content-hash", | ||
| "postinstall": "patch-package --patch-dir patches --patch-content-hash", |
There was a problem hiding this comment.
Using patch-package in a pnpm workspace is highly discouraged and can lead to issues. Since pnpm uses a global content-addressable store and hard links files into node_modules, running patch-package can directly modify files in the global store, potentially corrupting it for other projects on the same machine. Additionally, it may not apply reliably due to pnpm's symlink structure.
Instead, use pnpm's native patching mechanism:
- Run
pnpm patch @stencil/vue-output-targetto extract the package to a temporary directory. - Make the required changes in that directory.
- Run
pnpm patch-commit <temp-dir>to generate the patch file and automatically configure it underpatchedDependenciesin your configuration file (e.g.,pnpm-workspace.yamlorpackage.jsondepending on your pnpm version). - Remove
patch-packageandpostinstall-postinstallfrom yourdevDependencies.
| "prepare": "pnpm disable-telemetry && patch-package --patch-dir patches --patch-content-hash", | |
| "postinstall": "patch-package --patch-dir patches --patch-content-hash", | |
| "prepare": "pnpm disable-telemetry", |
There was a problem hiding this comment.
I've migrated away from patch-package to pnpm's native patching mechanism to stay consistent with the existing @stencil/core patch and avoid the content-addressable store risks you mentioned.
| * See https://github.com/vuejs/vue-next/issues/3889 | ||
| */ | ||
| const EMPTY_PROP = Symbol(); | ||
| const EMPTY_PROP = {}; |
There was a problem hiding this comment.
Since EMPTY_PROP is now defined as a plain object {} instead of a unique Symbol(), it is mutable. To prevent accidental mutation of this sentinel value elsewhere in the codebase, it is highly recommended to freeze the object using Object.freeze().
| const EMPTY_PROP = {}; | |
| const EMPTY_PROP = Object.freeze({}); |
There was a problem hiding this comment.
[EMPTY_PROP] is only used as a sentinel compared by reference equality [value !== EMPTY_PROP] and is never mutated anywhere, so freezing isn't functionally required. I've left it as a plain {} for now to keep the patch minimal and aligned with the upstream source.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/vue/src/vue-component-lib/utils.ts`:
- Around line 185-191: Add regression tests around the prop-forwarding logic
containing ARIA_PROP_PREFIX and EMPTY_PROP, covering omitted ARIA props being
excluded rather than passed as the sentinel string and explicit values such as
aria-label="Menu" being forwarded unchanged. Exercise the actual Vue runtime
path and the utility implementation when both are shipped.
In `@pnpm-workspace.yaml`:
- Line 28: Add a changeset describing the Vue wrapper accessibility behavior
change and identifying the affected package with the appropriate release bump;
do not treat the patch entry for `@stencil/vue-output-target` as sufficient
release documentation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 838d2ba2-7313-40e3-9e51-913f4aa4a83c
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
packages/angular-standalone-test-app/src/preview-examples/dropdown-button.csspackages/angular-test-app/src/preview-examples/dropdown-button.csspackages/angular/tsconfig.jsonpackages/html-test-app/src/preview-examples/dropdown-button.csspackages/nextjs-test-app/tsconfig.jsonpackages/react-test-app/src/preview-examples/dropdown-button.scoped.csspackages/vue-test-app/src/preview-examples/dropdown-button-icon.vuepackages/vue-test-app/src/preview-examples/dropdown-button.csspackages/vue-test-app/src/preview-examples/dropdown-button.vuepackages/vue/src/vue-component-lib/utils.tspackages/vue/tsconfig.jsonpatches/@stencil__vue-output-target.patchpnpm-workspace.yamltesting/visual-testing/tsconfig.test.json
| if ( | ||
| // eslint-disable-next-line no-prototype-builtins | ||
| (props.hasOwnProperty(key) && value !== EMPTY_PROP) || | ||
| key.startsWith(ARIA_PROP_PREFIX) | ||
| (key.startsWith(ARIA_PROP_PREFIX) && value !== EMPTY_PROP) | ||
| ) { | ||
| propsToAdd[key] = value; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add regression coverage for sentinel-valued and explicit ARIA props.
Test that omitted ARIA props never reach the custom element as a sentinel string, while explicit values such as aria-label="Menu" are still forwarded unchanged. Exercise the actual Vue runtime path as well as this utility if both implementations are shipped.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/vue/src/vue-component-lib/utils.ts` around lines 185 - 191, Add
regression tests around the prop-forwarding logic containing ARIA_PROP_PREFIX
and EMPTY_PROP, covering omitted ARIA props being excluded rather than passed as
the sentinel string and explicit values such as aria-label="Menu" being
forwarded unchanged. Exercise the actual Vue runtime path and the utility
implementation when both are shipped.
|
|
||
| patchedDependencies: | ||
| '@stencil/core': "patches/@stencil__core.patch" | ||
| '@stencil/vue-output-target': "patches/@stencil__vue-output-target.patch" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add a changeset for this accessibility behavior change.
This PR changes Vue wrapper behavior and user-facing accessibility output. Add a changeset for the affected package, or explicitly justify in the PR why the change is internal-only.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pnpm-workspace.yaml` at line 28, Add a changeset describing the Vue wrapper
accessibility behavior change and identifying the affected package with the
appropriate release bump; do not treat the patch entry for
`@stencil/vue-output-target` as sufficient release documentation.
Source: Path instructions
1 participant
💡 What is the current behavior?
GitHub Issue Number: #
Jira Issue Number: 4336
🆕 What is the new behavior?
🏁 Checklist
A pull request can only be merged if all of these conditions are met (where applicable):
pnpm test)pnpm lint)pnpm build, changes pushed)👨💻 Help & support
Summary by CodeRabbit
Bug Fixes
Compatibility