RD-T39 Bug fixes #75
RD-T39 Bug fixes #75
Conversation
📝 WalkthroughWalkthroughThis pull request centralizes web-specific input/select styles into new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 `@src/app/call/new/index.web.tsx`:
- Around line 1294-1318: The styles in webStyles remove the browser focus
indicator (outline: 'none') which breaks keyboard accessibility; remove outline:
'none' from webInput (and webSelect if present) and replace it with an
accessible custom focus style—either add new style keys (e.g., webInputFocus,
webSelectFocus) that set a visible outline/boxShadow and outlineOffset or
implement a :focus-visible rule in your CSS and apply a className to
inputs/selects so keyboard focus gets a clear visible ring while preserving
mouse appearance; update the components that use webStyles.webInput/webSelect to
apply the focus-class or focus style when focused.
♻️ Duplicate comments (1)
src/app/call/[id]/edit.web.tsx (1)
1194-1218: Same focus-outline concern as in the new-call page.
outline: 'none'removes the default focus indicator; please keep it or add a custom focus style.
| // Web-specific styles that use CSS-only properties | ||
| const webStyles: { [key: string]: React.CSSProperties } = { | ||
| webInput: { | ||
| width: '100%', | ||
| padding: 10, | ||
| paddingRight: 40, | ||
| fontSize: 14, | ||
| borderRadius: 8, | ||
| borderWidth: 1, | ||
| outline: 'none', | ||
| }, | ||
| webInputDisabled: { | ||
| opacity: 0.6, | ||
| cursor: 'not-allowed', | ||
| }, | ||
| webSelect: { | ||
| width: '100%', | ||
| padding: 10, | ||
| fontSize: 14, | ||
| borderRadius: 8, | ||
| borderWidth: 1, | ||
| outline: 'none', | ||
| cursor: 'pointer', | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Avoid removing focus outline without an alternative.
outline: 'none' removes the default focus indicator, which can make keyboard navigation inaccessible. Prefer keeping the default outline unless you add a custom focus style.
🔧 Suggested change (restore default focus indicator)
webInput: {
width: '100%',
padding: 10,
paddingRight: 40,
fontSize: 14,
borderRadius: 8,
borderWidth: 1,
- outline: 'none',
},
webSelect: {
width: '100%',
padding: 10,
fontSize: 14,
borderRadius: 8,
borderWidth: 1,
- outline: 'none',
cursor: 'pointer',
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Web-specific styles that use CSS-only properties | |
| const webStyles: { [key: string]: React.CSSProperties } = { | |
| webInput: { | |
| width: '100%', | |
| padding: 10, | |
| paddingRight: 40, | |
| fontSize: 14, | |
| borderRadius: 8, | |
| borderWidth: 1, | |
| outline: 'none', | |
| }, | |
| webInputDisabled: { | |
| opacity: 0.6, | |
| cursor: 'not-allowed', | |
| }, | |
| webSelect: { | |
| width: '100%', | |
| padding: 10, | |
| fontSize: 14, | |
| borderRadius: 8, | |
| borderWidth: 1, | |
| outline: 'none', | |
| cursor: 'pointer', | |
| }, | |
| }; | |
| // Web-specific styles that use CSS-only properties | |
| const webStyles: { [key: string]: React.CSSProperties } = { | |
| webInput: { | |
| width: '100%', | |
| padding: 10, | |
| paddingRight: 40, | |
| fontSize: 14, | |
| borderRadius: 8, | |
| borderWidth: 1, | |
| }, | |
| webInputDisabled: { | |
| opacity: 0.6, | |
| cursor: 'not-allowed', | |
| }, | |
| webSelect: { | |
| width: '100%', | |
| padding: 10, | |
| fontSize: 14, | |
| borderRadius: 8, | |
| borderWidth: 1, | |
| cursor: 'pointer', | |
| }, | |
| }; |
🤖 Prompt for AI Agents
In `@src/app/call/new/index.web.tsx` around lines 1294 - 1318, The styles in
webStyles remove the browser focus indicator (outline: 'none') which breaks
keyboard accessibility; remove outline: 'none' from webInput (and webSelect if
present) and replace it with an accessible custom focus style—either add new
style keys (e.g., webInputFocus, webSelectFocus) that set a visible
outline/boxShadow and outlineOffset or implement a :focus-visible rule in your
CSS and apply a className to inputs/selects so keyboard focus gets a clear
visible ring while preserving mouse appearance; update the components that use
webStyles.webInput/webSelect to apply the focus-class or focus style when
focused.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/app/call/new/index.web.tsx`:
- Around line 309-317: The submit handler leaves isSubmitting true when
validation fails for priority/type; update the handler (the function that calls
setIsSubmitting) to ensure setIsSubmitting(false) is invoked before each early
return on the priority/type checks (or move validation before calling
setIsSubmitting(true) or wrap the async flow in a try/finally that sets
isSubmitting false); specifically adjust the checks around priority and type in
index.web.tsx so that setIsSubmitting(false) is called (or validation is done
earlier) whenever you call toast.error(t('calls.invalid_priority')) or
toast.error(t('calls.invalid_type')) to avoid permanently disabling the submit
button.
♻️ Duplicate comments (1)
src/app/call/new/index.web.tsx (1)
1311-1333: Accessibility concern from previous review is now properly addressed.The
webStylesobject no longer containsoutline: 'none'. The focus removal is now applied locally inaccessibleInputStyles/accessibleSelectStyleswhile theweb-input-accessibleCSS class inglobal.cssprovides the visible:focus-visibleindicator for keyboard users.
🧹 Nitpick comments (2)
src/app/call/new/index.web.tsx (2)
119-126: Unnecessary type extension for CSS-in-JS selector.The
'&:focus-visible'?: React.CSSPropertiestype extension is CSS-in-JS syntax (styled-components/emotion) that has no effect on inline styles. Since focus styling is handled correctly via theweb-input-accessibleCSS class, this type can be simplified.🔧 Suggested simplification
// Add accessible focus styles for keyboard navigation const accessibleInputStyles = { ...inputStyles, outline: 'none', - } as React.CSSProperties & { - '&:focus-visible'?: React.CSSProperties; - }; + } as React.CSSProperties;
346-346: Remove unnecessaryrouterdependency.As flagged by static analysis,
routerfrom expo-router is a stable module export (outer scope value), not a reactive dependency. Including it doesn't cause bugs but generates linter warnings.🔧 Suggested fix
- [selectedLocation, callPriorities, callTypes, toast, t, router] + [selectedLocation, callPriorities, callTypes, toast, t]
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Summary by CodeRabbit
New Features
Style
Chores
✏️ Tip: You can customize this high-level summary in your review settings.