-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add blue focus on text inputs #8343
Conversation
d583c14
to
49d1a27
Compare
This reverts commit 3523c26.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
This PR implements consistent text input focus styling across the app settings while removing it from specific input types like phone/domain/array/email floating inputs.
- Added blue focus state with light blue box shadow in
TextInputV2.tsx
using theme colors - Increased default dropdown width from 160px to 200px across multiple components for better text accommodation
- Replaced
DropdownMenuInput
withTextInputV2
in view picker components to maintain consistent styling - Added automatic email continuation in
SignInUpForm.tsx
when no auth providers are available - Simplified input visual structure by making icons absolute positioned overlays in
TextInputV2.tsx
9 file(s) reviewed, 5 comment(s)
Edit PR Review Bot Settings | Greptile
if ( | ||
signInUpStep === SignInUpStep.Init && | ||
!authProviders.google && | ||
!authProviders.microsoft && | ||
!authProviders.sso | ||
) { | ||
continueWithEmail(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: This side effect should be moved to a useEffect hook to avoid potential render loop issues
&:focus { | ||
${({ theme }) => { | ||
return `box-shadow: 0px 0px 0px 3px ${RGBA(theme.color.blue, 0.1)}; | ||
border-color: ${theme.color.blue};`; | ||
}}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: The focus state uses a semi-transparent blue shadow (0.1 opacity) which may be too subtle for accessibility. Consider increasing the opacity for better visibility.
padding-left: ${({ theme, LeftIcon }) => | ||
LeftIcon ? `calc(${theme.spacing(4)} + 16px)` : theme.spacing(2)}; | ||
width: 100%; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: The left padding calculation assumes a fixed icon size of 16px. Consider using theme.icon.size.md instead for consistency.
@@ -57,7 +57,7 @@ describe('useDropdown', () => { | |||
wrapper: Wrapper, | |||
}); | |||
|
|||
expect(result.current.dropdownWidth).toBe(160); | |||
expect(result.current.dropdownWidth).toBe(200); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: default width changed from 160 to 200 - ensure this change is intentional and coordinated with the dropdownWidthComponentState default value
<TextInputV2 | ||
value={viewPickerInputName} | ||
onChange={(event) => { | ||
onChange={(value) => { | ||
setViewPickerIsDirty(true); | ||
setViewPickerInputName(event.target.value); | ||
setViewPickerInputName(value); | ||
}} | ||
autoFocus | ||
/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: TextInputV2 may need a placeholder prop to match previous DropdownMenuInput behavior
We removed the blue focus on phone/domain/array/email floating inputs but we want to keep it in the app settings.