refactor(toggle): migrate Toggle from Flow to TypeScript - #4764
refactor(toggle): migrate Toggle from Flow to TypeScript#4764bonchevskyi wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe PR adds TypeScript and Flow-compatible ChangesToggle migration
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 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: 2
🧹 Nitpick comments (1)
src/components/toggle/__tests__/Toggle.test.tsx (1)
8-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse typed props in both migrated test helpers.
Both helpers use
any, so TypeScript does not validate the public component contracts.
src/components/toggle/__tests__/Toggle.test.tsx#L8-L9: usePartial<ToggleProps>.src/components/toggle/__tests__/ToggleField.test.tsx#L7-L8: use a fixture type based onToggleFieldPropsthat requiresfield.🤖 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 `@src/components/toggle/__tests__/Toggle.test.tsx` around lines 8 - 9, Replace the any-typed getWrapper props in src/components/toggle/__tests__/Toggle.test.tsx lines 8-9 with Partial<ToggleProps>. In src/components/toggle/__tests__/ToggleField.test.tsx lines 7-8, type the helper fixture from ToggleFieldProps while requiring the field property, so both migrated test helpers validate their component contracts.
🤖 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 `@src/components/toggle/Toggle.js.flow`:
- Around line 29-31: Update the onMouseEnter and onMouseLeave callback
declarations in the Toggle prop types to use SyntheticMouseEvent<HTMLDivElement>
instead of SyntheticInputEvent<HTMLDivElement>, matching the container div
handlers while preserving their existing callback signatures.
In `@src/components/toggle/ToggleField.tsx`:
- Around line 9-16: Remove the Formik meta bag before forwarding props from
ToggleField. In src/components/toggle/ToggleField.tsx lines 9-16, destructure
and discard meta alongside form; apply the same change in
src/components/toggle/ToggleField.js.flow lines 11-13 so both wrappers prevent
meta from reaching TogglePrimitive and the native input.
---
Nitpick comments:
In `@src/components/toggle/__tests__/Toggle.test.tsx`:
- Around line 8-9: Replace the any-typed getWrapper props in
src/components/toggle/__tests__/Toggle.test.tsx lines 8-9 with
Partial<ToggleProps>. In src/components/toggle/__tests__/ToggleField.test.tsx
lines 7-8, type the helper fixture from ToggleFieldProps while requiring the
field property, so both migrated test helpers validate their component
contracts.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2061a3ba-9108-481a-a541-7ede3c409e45
⛔ Files ignored due to path filters (2)
src/components/toggle/__tests__/__snapshots__/Toggle.test.tsx.snapis excluded by!**/*.snapsrc/components/toggle/__tests__/__snapshots__/ToggleField.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (9)
src/components/toggle/Toggle.js.flowsrc/components/toggle/Toggle.stories.tsxsrc/components/toggle/Toggle.tsxsrc/components/toggle/ToggleField.js.flowsrc/components/toggle/ToggleField.tsxsrc/components/toggle/__tests__/Toggle.test.tsxsrc/components/toggle/__tests__/ToggleField.test.tsxsrc/components/toggle/index.js.flowsrc/components/toggle/index.ts
💤 Files with no reviewable changes (1)
- src/components/toggle/Toggle.stories.tsx
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/components/toggle/__tests__/Toggle.test.tsx (1)
8-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse typed props in both migrated test helpers.
Both helpers use
any, so TypeScript does not validate the public component contracts.
src/components/toggle/__tests__/Toggle.test.tsx#L8-L9: usePartial<ToggleProps>.src/components/toggle/__tests__/ToggleField.test.tsx#L7-L8: use a fixture type based onToggleFieldPropsthat requiresfield.🤖 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 `@src/components/toggle/__tests__/Toggle.test.tsx` around lines 8 - 9, Replace the any-typed getWrapper props in src/components/toggle/__tests__/Toggle.test.tsx lines 8-9 with Partial<ToggleProps>. In src/components/toggle/__tests__/ToggleField.test.tsx lines 7-8, type the helper fixture from ToggleFieldProps while requiring the field property, so both migrated test helpers validate their component contracts.
🤖 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 `@src/components/toggle/Toggle.js.flow`:
- Around line 29-31: Update the onMouseEnter and onMouseLeave callback
declarations in the Toggle prop types to use SyntheticMouseEvent<HTMLDivElement>
instead of SyntheticInputEvent<HTMLDivElement>, matching the container div
handlers while preserving their existing callback signatures.
In `@src/components/toggle/ToggleField.tsx`:
- Around line 9-16: Remove the Formik meta bag before forwarding props from
ToggleField. In src/components/toggle/ToggleField.tsx lines 9-16, destructure
and discard meta alongside form; apply the same change in
src/components/toggle/ToggleField.js.flow lines 11-13 so both wrappers prevent
meta from reaching TogglePrimitive and the native input.
---
Nitpick comments:
In `@src/components/toggle/__tests__/Toggle.test.tsx`:
- Around line 8-9: Replace the any-typed getWrapper props in
src/components/toggle/__tests__/Toggle.test.tsx lines 8-9 with
Partial<ToggleProps>. In src/components/toggle/__tests__/ToggleField.test.tsx
lines 7-8, type the helper fixture from ToggleFieldProps while requiring the
field property, so both migrated test helpers validate their component
contracts.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2061a3ba-9108-481a-a541-7ede3c409e45
⛔ Files ignored due to path filters (2)
src/components/toggle/__tests__/__snapshots__/Toggle.test.tsx.snapis excluded by!**/*.snapsrc/components/toggle/__tests__/__snapshots__/ToggleField.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (9)
src/components/toggle/Toggle.js.flowsrc/components/toggle/Toggle.stories.tsxsrc/components/toggle/Toggle.tsxsrc/components/toggle/ToggleField.js.flowsrc/components/toggle/ToggleField.tsxsrc/components/toggle/__tests__/Toggle.test.tsxsrc/components/toggle/__tests__/ToggleField.test.tsxsrc/components/toggle/index.js.flowsrc/components/toggle/index.ts
💤 Files with no reviewable changes (1)
- src/components/toggle/Toggle.stories.tsx
🛑 Comments failed to post (1)
src/components/toggle/Toggle.js.flow (1)
29-31: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect existing Flow mouse-handler type conventions before updating this API. rg -n -C 2 --glob '*.js.flow' \ 'SyntheticMouseEvent<HTMLDivElement>|onMouseEnter.*Synthetic|onMouseLeave.*Synthetic' \ srcRepository: box/box-ui-elements
Length of output: 856
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "## Toggle.js.flow" cat -n src/components/toggle/Toggle.js.flow | sed -n '1,110p' echo echo "## Toggle.tsx relevant event props/usages" if [ -f src/components/toggle/Toggle.tsx ]; then cat -n src/components/toggle/Toggle.tsx | sed -n '1,160p' else fd -a 'Toggle.tsx$' . fi echo echo "## Flow event declarations in repo" rg -n --glob '*.js.flow' --glob 'flow-typed/**' 'SyntheticMouseEvent|SyntheticInputEvent|type SyntheticMouseEvent|type SyntheticInputEvent' site src flow-typed 2>/dev/null | head -200Repository: box/box-ui-elements
Length of output: 9701
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' from pathlib import Path for p in Path('.').rglob('*.js.flow'): text = p.read_text(errors='ignore') if 'SyntheticInputEvent<HTMLDivElement>' in text or 'SyntheticMouseEvent<HTMLDivElement>' in text: print(p) for i,line in enumerate(text.splitlines(), 1): if 'SyntheticInputEvent<HTMLDivElement>' in line or 'SyntheticMouseEvent<HTMLDivElement>' in line: print(f' {i}: {line.strip()}') PYRepository: box/box-ui-elements
Length of output: 334
Use mouse event types for the container handlers.
onMouseEnterandonMouseLeaveare attached to the toggle container<div>at line 71, so Flow consumers receive mouse events, not input events. Change both declarations toSyntheticMouseEvent<HTMLDivElement>.🤖 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 `@src/components/toggle/Toggle.js.flow` around lines 29 - 31, Update the onMouseEnter and onMouseLeave callback declarations in the Toggle prop types to use SyntheticMouseEvent<HTMLDivElement> instead of SyntheticInputEvent<HTMLDivElement>, matching the container div handlers while preserving their existing callback signatures.
c6e3fbb to
2c9ff5b
Compare
2c9ff5b to
2748d5e
Compare
| describe('components/toggle/Toggle', () => { | ||
| const getWrapper = (props = {}) => | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| const getWrapper = (props: any = {}) => |
There was a problem hiding this comment.
q: Can we use ToggleProps instead of any here?
There was a problem hiding this comment.
Tried Partial<ToggleProps> but it blows up on the data-resin-target case, which is there to check rest forwarding. Left any so that test still type-checks.
|
|
||
| describe('components/toggle/ToggleField', () => { | ||
| const getWrapper = (props = {}) => shallow(<ToggleField {...props} />); | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any |
There was a problem hiding this comment.
q: Can we use ToggleFieldProps instead of any here?
There was a problem hiding this comment.
Not really, not without rewriting the fixtures. These tests pass a partial field (onBlur/onChange as strings, no form/meta), which doesn’t satisfy ToggleFieldProps. Same reason the TextInputField tests still use any.
| // eslint-disable-next-line @typescript-eslint/no-unused-vars -- strip Formik form and meta bags from forwarded props | ||
| form, | ||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars -- strip Formik form and meta bags from forwarded props |
There was a problem hiding this comment.
q: Should we pass form and meta if we do not use them in component?
There was a problem hiding this comment.
We still have to accept them because Formik injects field / form / meta, but we don’t pass form or meta through. They’re pulled off so they don’t end up on Toggle / the input. field is the one we actually spread.
2 participants
Convert Toggle component to TypeScript
This PR converts
src/components/togglefrom JavaScript with Flow to TypeScript.Changes
Toggle.jstoToggle.tsxwith exportedTogglePropsinterfaceToggleField.jstoToggleField.tsxwith exportedToggleFieldPropsinterfaceindex.jstoindex.ts, re-exporting the components and their typesToggle.stories.jstoToggle.stories.tsx__tests__/Toggle.test.jsand__tests__/ToggleField.test.jsto.test.tsx.js.flowfiles for backward compatibilityTesting
src/components/toggle; all 10 pass with snapshots matching previous outputyarn lint:tsandflow checkpassSummary by CodeRabbit
New Features
Tests