Patch added to handle the reactive classes or dynamic classes handlin… - #2678
Patch added to handle the reactive classes or dynamic classes handlin…#2678khathija-ahamadi wants to merge 1 commit into
Conversation
|
✅ Deploy Preview for ix-storybook canceled.
|
📝 WalkthroughWalkthroughThe PR patches Stencil Vue runtime class synchronization in ESM and CommonJS builds, registers the patch through the pnpm workspace, and documents the patched dependency and its purpose. ChangesStencil Vue class synchronization
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 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 |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@patches/`@stencil__vue-output-target.patch:
- Around line 8-10: Add regression tests around getComponentClasses and the
affected runtime bundles covering reactive class removal, preservation of
externally or component-added ix-invalid, and empty class bindings. Verify
classes are correctly owned and removed without deleting unrelated classes,
while preserving existing behavior for valid class strings.
In `@patches/README.md`:
- Around line 1-3: Update the pull request title to follow the required
conventional format, using a type, optional scope, colon, and concise
description; for example, use a format like “fix(vue): patch reactive class
synchronization.”
In `@pnpm-workspace.yaml`:
- Line 28: Add a changeset for the package that ships the Vue wrappers,
describing the consumer-visible behavior fix and selecting the appropriate
release level. If this patch is genuinely internal-only, provide an explicit
justification instead of adding the changeset.
🪄 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 Plus
Run ID: fd1b8c0a-dcd2-489f-90c9-e090e542af40
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
patches/@stencil__vue-output-target.patchpatches/README.mdpnpm-workspace.yaml
| const getComponentClasses = (classes) => { | ||
| - return classes?.split(' ') || []; | ||
| + return (classes?.split(' ') || []).filter(Boolean); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add regression coverage for class ownership and removal.
This changes runtime behavior in both bundles, but no regression test is included. Cover reactive class removal, preservation of externally/component-added ix-invalid, and empty class bindings.
As per path instructions: “Prioritize correctness, regressions, accessibility, release impact, and missing validation.”
Also applies to: 20-27, 38-40, 50-57
🤖 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 `@patches/`@stencil__vue-output-target.patch around lines 8 - 10, Add
regression tests around getComponentClasses and the affected runtime bundles
covering reactive class removal, preservation of externally or component-added
ix-invalid, and empty class bindings. Verify classes are correctly owned and
removed without deleting unrelated classes, while preserving existing behavior
for valid class strings.
Source: Path instructions
There was a problem hiding this comment.
@nuke-ellington
I will add tests once initial review is done and go ahead with the solution is given.
Manual tests in all frameworks is carried out and the solution is working as expected.
| # Stencil Patches | ||
|
|
||
| ## Vue Output Target |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use a conventional PR title.
The current title is a prose sentence rather than <type>[optional scope]: <description>; use a title such as fix(vue): patch reactive class synchronization. (github.com)
As per path instructions: “Expect title format: [optional ]: .”
🤖 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 `@patches/README.md` around lines 1 - 3, Update the pull request title to
follow the required conventional format, using a type, optional scope, colon,
and concise description; for example, use a format like “fix(vue): patch
reactive class synchronization.”
Sources: Path instructions, MCP tools
|
|
||
| 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 the required changeset for this consumer-visible fix.
This patch changes shipped Vue behavior, but no changeset is included. Add one for the package that ships the Vue wrappers, or explicitly justify why this is internal-only.
As per path instructions: “Changesets are required for public API updates, behavior changes, styling/theming changes, accessibility changes, and bug fixes with user impact; if no changeset is included, require the author to explicitly justify 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 for the package that ships
the Vue wrappers, describing the consumer-visible behavior fix and selecting the
appropriate release level. If this patch is genuinely internal-only, provide an
explicit justification instead of adding the changeset.
Source: Path instructions
1 participant
💡 What is the current behavior?
The Input field Invalid State post inputting invalid input reappears even after removing the invalid input and start entering valid input.
GitHub Issue Number: #2617
🆕 What is the new behavior?
Fixes the issue by adding a patch file where any control like ix-input incorrectly removed a user-provided ix-invalid class on load when the invalidText prop wasn't set. The class is now only auto-removed if it was previously added by the component itself, so manually-applied validation classes are preserved as before.
🏁 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
Documentation