fix: pass data attributes to withUniwind#477
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughgetWebStyles gained a componentProps parameter used to populate a temporary off‑DOM dataset; HOCs and hooks pass component props and inject generateDataSet output into wrapped components; dataset typing widened; tests updated for data-attribute handling; root devDependency Changes
Sequence Diagram(s)sequenceDiagram
participant Comp as Component
participant HOC as withUniwind HOC
participant Gen as generateDataSet
participant Styles as getWebStyles
participant DOM as Off‑DOM Dummy Element
Comp->>HOC: render(props, className)
HOC->>Gen: generateDataSet(props)
Gen-->>HOC: dataSet
HOC->>Styles: getWebStyles(className, props, context)
Styles->>DOM: set dataset entries (from props)
Styles->>DOM: compute active CSS rules
DOM-->>Styles: matched rules / computed styles
Styles->>DOM: remove temporary dataset entries
Styles-->>HOC: resolved RN styles
HOC-->>Comp: pass generatedProps (including dataSet)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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.
🧹 Nitpick comments (3)
packages/uniwind/src/core/web/getWebStyles.ts (1)
55-59:componentPropsparameter is accepted but unused.The native implementation in
store.tsvalidates data-attribute requirements againstcomponentPropsviavalidateDataAttributes(style.dataAttributes, componentProps), skipping styles when requirements aren't met.The web implementation accepts the same parameter for API consistency but doesn't use it. On web, this likely works because the
dataSetprop is forwarded to the DOM element, allowing browser CSS selectors (e.g.,[data-test]) to match naturally.Consider adding a brief comment explaining why
componentPropsis intentionally unused on web to prevent future confusion or accidental removal.📝 Proposed documentation
export const getWebStyles = ( className: string | undefined, + // componentProps is accepted for API consistency with native but unused on web. + // Web relies on browser CSS selector matching via the dataSet prop instead of runtime validation. componentProps: Record<string, unknown> | undefined, uniwindContext: UniwindContextType, ): RNStyle => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/uniwind/src/core/web/getWebStyles.ts` around lines 55 - 59, The parameter componentProps on getWebStyles is accepted but intentionally unused; add a concise inline comment inside getWebStyles explaining that componentProps is preserved for API parity with the native implementation (see validateDataAttributes in store.ts) and that on web data-attributes are handled by forwarding dataSet to the DOM and matched by CSS selectors, so no runtime validation is required here—this prevents future maintainers from removing the parameter.packages/uniwind/tests/web/hoc/withUniwind.test.tsx (1)
35-54: Consider adding a test fordataSetprop injection.The HOC now injects
dataSet={generateDataSet(props)}, but there's no test verifying this behavior on web. Consider adding a test that confirmsdata-*props are converted todataSetand passed to the wrapped component.💡 Example test case
test('[auto] Should convert data-* props to dataSet', () => { ComponentWithSpy.mockClear() const AutoWithUniwind = withUniwind(ComponentWithSpy) render(<AutoWithUniwind data-active={true} data-state="open" testID="test-component" />) const receivedProps = ComponentWithSpy.mock.calls[0][0] expect(receivedProps.dataSet).toEqual({ active: true, state: 'open', }) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/uniwind/tests/web/hoc/withUniwind.test.tsx` around lines 35 - 54, Add a test that verifies withUniwind injects a dataSet prop produced by generateDataSet: clear ComponentWithSpy mock, create AutoWithUniwind = withUniwind(ComponentWithSpy), render AutoWithUniwind with data-active and data-state (and testID), then inspect ComponentWithSpy.mock.calls[0][0] and assert receivedProps.dataSet equals { active: true, state: 'open' }; use the same render/test utilities and access pattern as the existing tests so it mirrors the color mapping test.packages/uniwind/tests/e2e/getWebStyles.test.ts (1)
66-84: Consider adding e2e tests for data-attribute selectors.Given that the PR objective is to fix
data-*attribute selectors forwithUniwind, it would be valuable to add e2e tests verifying that classes likedata-active:bg-red-500resolve correctly when the corresponding data attribute is present on the element.Would you like me to draft test cases for data-attribute selector scenarios?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/uniwind/tests/e2e/getWebStyles.test.ts` around lines 66 - 84, Add e2e tests in the same suite (packages/uniwind/tests/e2e/getWebStyles.test.ts) that mirror the existing pattern but cover data-attribute selectors: call getWebStyles(page, 'data-active:bg-red-500') against an element that has the corresponding data-active attribute (use withUniwind behavior/setup) and assert the returned styles include backgroundColor: TW_RED_500; also add a negative test where the element lacks the data-active attribute and expect an empty object. Reference getWebStyles and the data-attribute variant 'data-active:bg-red-500' when adding these cases so they run alongside the existing bg-red-500/text-base tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/uniwind/src/core/web/getWebStyles.ts`:
- Around line 55-59: The parameter componentProps on getWebStyles is accepted
but intentionally unused; add a concise inline comment inside getWebStyles
explaining that componentProps is preserved for API parity with the native
implementation (see validateDataAttributes in store.ts) and that on web
data-attributes are handled by forwarding dataSet to the DOM and matched by CSS
selectors, so no runtime validation is required here—this prevents future
maintainers from removing the parameter.
In `@packages/uniwind/tests/e2e/getWebStyles.test.ts`:
- Around line 66-84: Add e2e tests in the same suite
(packages/uniwind/tests/e2e/getWebStyles.test.ts) that mirror the existing
pattern but cover data-attribute selectors: call getWebStyles(page,
'data-active:bg-red-500') against an element that has the corresponding
data-active attribute (use withUniwind behavior/setup) and assert the returned
styles include backgroundColor: TW_RED_500; also add a negative test where the
element lacks the data-active attribute and expect an empty object. Reference
getWebStyles and the data-attribute variant 'data-active:bg-red-500' when adding
these cases so they run alongside the existing bg-red-500/text-base tests.
In `@packages/uniwind/tests/web/hoc/withUniwind.test.tsx`:
- Around line 35-54: Add a test that verifies withUniwind injects a dataSet prop
produced by generateDataSet: clear ComponentWithSpy mock, create AutoWithUniwind
= withUniwind(ComponentWithSpy), render AutoWithUniwind with data-active and
data-state (and testID), then inspect ComponentWithSpy.mock.calls[0][0] and
assert receivedProps.dataSet equals { active: true, state: 'open' }; use the
same render/test utilities and access pattern as the existing tests so it
mirrors the color mapping test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 34242f46-794b-4342-9af0-106a67bd0007
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
package.jsonpackages/uniwind/src/core/web/getWebStyles.tspackages/uniwind/src/hoc/withUniwind.native.tsxpackages/uniwind/src/hoc/withUniwind.tsxpackages/uniwind/src/hooks/useResolveClassNames.tspackages/uniwind/tests/e2e/getWebStyles.test.tspackages/uniwind/tests/native/hoc/withUniwind.test.tsxpackages/uniwind/tests/web/hoc/withUniwind.test.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/uniwind/src/core/web/getWebStyles.ts`:
- Around line 77-91: The current loop that writes attributes using dummy.dataset
(from generateDataSet(componentProps)) fails for hyphenated keys and omits
aria-* attributes, and cleanup can be skipped if
getActiveStylesForClass(className) throws; replace the dataset assignment with
element.setAttribute(key, String(value)) and ensure you also include any aria-*
entries from generateDataSet, and wrap the call to
getActiveStylesForClass(className) in a try/finally so that in finally you
remove the same attributes via dummy.removeAttribute(key) (use the same keys you
set) to guarantee cleanup even on exceptions.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: eb60fca8-1006-46e0-b979-a23a8938b9e6
📒 Files selected for processing (2)
packages/uniwind/src/components/web/generateDataSet.tspackages/uniwind/src/core/web/getWebStyles.ts
✅ Files skipped from review due to trivial changes (1)
- packages/uniwind/src/components/web/generateDataSet.ts
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Autofix skipped. No unresolved CodeRabbit review comments with fix instructions found. |
Contributor
|
🚀 This pull request is included in v1.6.1. See Release v1.6.1 for release notes. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fixes #475
Summary by CodeRabbit
Refactor
Tests
Chores