Add support for HTML attributes on buttons and cancelIcon#3370
Add support for HTML attributes on buttons and cancelIcon#3370RobbieTheWagner merged 3 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
Coverage Impact This PR will not change total coverage. 🚦 See full report on Qlty Cloud »🛟 Help
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
shepherd.js/src/components/shepherd-cancel-icon.ts (1)
17-17:??is more semantically precise than||here.Since
attrsis typed asRecord<string, string | number | boolean> | undefined, it can never be falsy except whenundefined/null. Using||works correctly but implies falsiness checking, whereas??(nullish coalescing) expresses the true intent. The same applies toshepherd-button.tsline 25.♻️ Suggested change
- ...(cancelIcon.attrs || {}), + ...(cancelIcon.attrs ?? {}),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shepherd.js/src/components/shepherd-cancel-icon.ts` at line 17, Replace the use of the logical OR fallback with the nullish coalescing operator when spreading attribute objects: change the spread from ...(cancelIcon.attrs || {}) to ...(cancelIcon.attrs ?? {}) in shepherd-cancel-icon (referencing the cancelIcon.attrs symbol), and make the analogous change in shepherd-button.ts at the place using attrs (line referencing the attrs spread) so that only null/undefined falls back to an empty object rather than any falsy value.shepherd.js/test/unit/components/shepherd-button.spec.js (1)
180-213: Test gap: action-less button withonclickinattrsis untested.The "does not override core button attributes" test supplies a real
action, soonclick: action(a non-null function) is always set byh. The untested edge case (tied to the concern in the component) is a button withaction: undefinedandattrs: { onclick: 'someCode' }, whereonclick: nullis passed as the core value andhmay skip clearing it.🧪 Proposed additional test
+ it('does not persist onclick from attrs on action-less buttons', () => { + const button = createShepherdButton( + { + attrs: { onclick: 'alert("unexpected")' } + }, + undefined + ); + container.appendChild(button); + + // The string onclick attribute should not survive; no inline handler + expect(button.onclick).toBeNull(); + expect(button.getAttribute('onclick')).toBeNull(); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shepherd.js/test/unit/components/shepherd-button.spec.js` around lines 180 - 213, Add a new unit test for createShepherdButton that covers the edge case where action is undefined but attrs includes an onclick string: call createShepherdButton with action: undefined (or omitted) and attrs: { onclick: 'someCode', 'data-test': 'x' } and a mockStep, append to container, then assert the core onclick is not set on the rendered button (e.g., button.getAttribute('onclick') is null or button).not.toHaveAttribute('onclick'), while still verifying other core attributes (type, class includes 'shepherd-button', disabled/tabindex/aria-label) remain protected and non-conflicting custom attrs (like data-test) are applied; place this alongside the existing "does not override core button attributes" test to ensure the h/createShepherdButton path that clears onclick behaves correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs-src/src/content/docs/guides/usage.md`:
- Around line 260-262: The docs add a broken link to /guides/custom-attributes;
fix by either adding the missing guide file or removing/adjusting the link and
referencing the existing JSDoc for StepOptionsButton.attrs instead—update the
usage.md line that links `[Custom Attributes guide](/guides/custom-attributes)`
to point to the inline documentation (mention StepOptionsButton.attrs JSDoc) or
replace the link with a valid path to a newly created custom-attributes.md guide
so the reference resolves.
---
Nitpick comments:
In `@shepherd.js/src/components/shepherd-cancel-icon.ts`:
- Line 17: Replace the use of the logical OR fallback with the nullish
coalescing operator when spreading attribute objects: change the spread from
...(cancelIcon.attrs || {}) to ...(cancelIcon.attrs ?? {}) in
shepherd-cancel-icon (referencing the cancelIcon.attrs symbol), and make the
analogous change in shepherd-button.ts at the place using attrs (line
referencing the attrs spread) so that only null/undefined falls back to an empty
object rather than any falsy value.
In `@shepherd.js/test/unit/components/shepherd-button.spec.js`:
- Around line 180-213: Add a new unit test for createShepherdButton that covers
the edge case where action is undefined but attrs includes an onclick string:
call createShepherdButton with action: undefined (or omitted) and attrs: {
onclick: 'someCode', 'data-test': 'x' } and a mockStep, append to container,
then assert the core onclick is not set on the rendered button (e.g.,
button.getAttribute('onclick') is null or
button).not.toHaveAttribute('onclick'), while still verifying other core
attributes (type, class includes 'shepherd-button',
disabled/tabindex/aria-label) remain protected and non-conflicting custom attrs
(like data-test) are applied; place this alongside the existing "does not
override core button attributes" test to ensure the h/createShepherdButton path
that clears onclick behaves correctly.
1 participant
Summary by CodeRabbit
New Features
Documentation
Tests