Add Data Attributes for CancelIcon and Button components #3333
Conversation
…Component/CancelIconComponent to use it
|
@hlopez94 is attempting to deploy a commit to the shipshapecode Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughAdds a utility to convert DataAttribute arrays into HTML data-* attributes, applies it to Shepherd button and cancel-icon components (spreading resulting attributes onto buttons), and introduces comprehensive unit tests covering conversion and component rendering for various dataAttribute cases. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (1)
✏️ Tip: You can disable this entire section by setting 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: 1
🤖 Fix all issues with AI agents
In `@test/unit/components/shepherd-cancel-icon.spec.js`:
- Around line 55-73: The assertion in the test "renders close symbol (×)"
is wrong because textContent returns the decoded character; update the
expectation for the queried element (closeSymbol from
container.querySelector('span[aria-hidden="true"]')) to assert the actual
multiplication sign character (×) instead of the literal "×". If the
component is supposed to render the raw entity text rather than the decoded
symbol, change the assertion to check innerHTML (closeSymbol.innerHTML) for the
string "×" instead.
🧹 Nitpick comments (2)
shepherd.js/src/utils/data-attributes.ts (1)
25-37: Consider validating attribute id format.The implementation is clean and handles edge cases well. However, there's no validation that
attr.idcontains valid characters for a data attribute name. HTML5 data-* attribute names must not contain uppercase letters and should follow specific naming conventions.For example, an
idwith spaces like"my attr"would producedata-my attrwhich is invalid HTML.💡 Optional: Add id format validation
return dataAttributes.reduce((acc, attr) => { - if (attr.id) { + if (attr.id && /^[a-z][a-z0-9-]*$/i.test(attr.id)) { acc[`data-${attr.id}`] = String(attr.value); } return acc; }, {} as Record<string, string>);Alternatively, you could sanitize the id by converting to lowercase and replacing invalid characters, or simply document that consumers are responsible for valid ids.
test/unit/components/shepherd-cancel-icon.spec.js (1)
416-435: Weak assertion doesn't verify transparent background.The test name says "button has transparent background" but the assertion
expect(cancelIcon.style.background || styles.background).toBeTruthy()only checks that some background value exists, not that it's transparent. This test would pass for any background value including opaque colors.Consider either:
- Removing this test if styling is tested elsewhere (CSS unit tests)
- Making the assertion more specific if transparency verification is important
Option: Remove or improve the assertion
If you want to actually test for transparency:
const cancelIcon = container.querySelector('.shepherd-cancel-icon'); const styles = window.getComputedStyle(cancelIcon); - // Basic style checks - specific values might vary based on CSS - expect(cancelIcon.style.background || styles.background).toBeTruthy(); + // Verify transparent or no background + expect(styles.backgroundColor).toMatch(/transparent|rgba\(0,\s*0,\s*0,\s*0\)/);Or remove the test if CSS is validated elsewhere:
- it('button has transparent background', () => { - // ... entire test ... - });
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
shepherd.js/src/components/shepherd-button.svelteshepherd.js/src/components/shepherd-cancel-icon.svelteshepherd.js/src/utils/data-attributes.tstest/unit/components/shepherd-button.spec.jstest/unit/components/shepherd-cancel-icon.spec.jstest/unit/components/shepherd-header.spec.jstest/unit/utils/data-attributes.spec.js
🧰 Additional context used
🧬 Code graph analysis (3)
test/unit/components/shepherd-header.spec.js (1)
test/unit/test-helpers.js (1)
container(6-6)
test/unit/utils/data-attributes.spec.js (1)
shepherd.js/src/utils/data-attributes.ts (1)
convertDataAttributes(25-38)
test/unit/components/shepherd-cancel-icon.spec.js (2)
shepherd.js/src/tour.ts (1)
Tour(106-470)shepherd.js/src/step.ts (1)
Step(305-727)
🔇 Additional comments (18)
shepherd.js/src/utils/data-attributes.ts (1)
1-7: LGTM!The
DataAttributeinterface is well-defined with appropriate types for thevaluefield supporting strings, numbers, and booleans.shepherd.js/src/components/shepherd-button.svelte (1)
3-3: LGTM!Clean integration of the data attributes feature. The use of
$derivedensures reactivity, and spreading the attributes at the end of the button element is the correct approach.Also applies to: 22-23, 35-35
shepherd.js/src/components/shepherd-cancel-icon.svelte (1)
2-2: LGTM!This directly addresses issue
#2036. The implementation is consistent with the button component, and correctly sourcesdataAttributesfrom thecancelIconconfig object alongside the existinglabelproperty.Also applies to: 14-14, 22-22
test/unit/utils/data-attributes.spec.js (1)
1-123: Excellent test coverage!The test suite comprehensively covers:
- Null/undefined/empty inputs
- Single and multiple attributes
- Type coercion for numbers and booleans
- Invalid inputs (missing/empty id)
- Edge cases (zero, empty string, special characters)
- Duplicate handling behavior
This provides strong confidence in the utility's behavior.
test/unit/components/shepherd-button.spec.js (1)
197-341: LGTM!Comprehensive integration tests that verify the data attributes feature works correctly at the component level. The tests smartly verify:
- Single and multiple attributes render correctly
- Type coercion works in the DOM
- Empty/undefined inputs don't break the component
- Invalid inputs are filtered out
- Feature works alongside existing button properties
Good use of attribute filtering (lines 269-272, 309-312) to verify exact attribute counts.
test/unit/components/shepherd-header.spec.js (5)
92-114: LGTM!Good test coverage for the new
dataAttributesfeature on the cancel icon. The test properly validates that multiple data attributes are correctly applied to the element.
116-140: LGTM!Solid integration test verifying that both
labelanddataAttributeswork correctly together, ensuring no conflicts between existing and new functionality.
142-174: LGTM!Title rendering tests are clear and cover both positive and negative cases appropriately.
176-198: LGTM!Good integration test ensuring title and cancel icon render correctly when both are provided.
200-217: LGTM!Appropriate structural test verifying the header element's CSS class and semantic HTML tag.
test/unit/components/shepherd-cancel-icon.spec.js (8)
10-31: LGTM!Good basic functionality tests covering default aria-label and button type attributes.
32-53: LGTM!Properly tests custom aria-label override functionality.
76-125: LGTM!Click behavior tests are well-implemented, including proper async handling and verification of
preventDefaultbeing called.
127-176: LGTM!Excellent coverage of single and multiple data attributes with proper assertions.
178-228: LGTM!Good tests for type coercion of numeric and boolean values to strings, which is the expected HTML data attribute behavior.
230-298: LGTM!Comprehensive edge case coverage for empty, undefined, and null
dataAttributesinputs.
300-361: LGTM!Good test coverage for invalid entries (missing/empty id) and special characters in values.
364-394: LGTM!Good integration test verifying label and dataAttributes work together correctly.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
1 participant
This pull request adds support for custom
data-*attributes to the Shepherd tour button and cancel icon components, allowing users to pass arbitrary data attributes via configuration. It introduces a new utility function to handle conversion of these attributes and includes comprehensive unit tests to ensure correct behavior across various scenarios.Component enhancements:
data-*attributes on theshepherd-buttonandshepherd-cancel-iconcomponents by using a newconvertDataAttributesutility. This enables passing attributes likedata-test,data-step, etc., via the config. [1] [2] [3] [4] [5]Utility function:
convertDataAttributesindata-attributes.ts, which converts an array of{ id, value }objects into a dictionary ofdata-*attributes for easy spreading onto HTML elements. Handles string, number, boolean, and edge cases.Testing:
shepherd-buttonto verify correct application of single, multiple, numeric, boolean, and edge-case data attributes.shepherd-headerto verify that the cancel icon correctly renders with custom data attributes and label combinations.Closes #2036
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.