-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
fix: Revert "Revert "feat: Added focus ring for focus visible (#37700)" (#… #38655
Changes from 12 commits
0cec0af
f8bb57f
2ff3d4b
271ff45
bc6028e
a743c64
734fd4a
2f6c1d6
4d8d3f7
16642bd
38905b5
488f21c
d3cbfce
7b48b1f
9b5c755
e6a4968
7f7dcd2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Is this expected? |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,4 @@ | ||
import React, { forwardRef } from "react"; | ||
import { useFocusRing } from "@react-aria/focus"; | ||
import React, { forwardRef, useCallback } from "react"; | ||
import { useTextField } from "@react-aria/textfield"; | ||
import clsx from "classnames"; | ||
|
||
|
@@ -55,7 +54,7 @@ const Input = forwardRef<HTMLInputElement, InputProps>( | |
const { descriptionProps, errorMessageProps, inputProps, labelProps } = | ||
// @ts-expect-error fix this the next time the file is edited | ||
useTextField(props, inputRef); | ||
const { focusProps, isFocusVisible } = useFocusRing(); | ||
|
||
const { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix the type assertion. The @ts-expect-error comment indicates a known type issue that needs to be addressed. Consider fixing the type issue by:
|
||
className: startIconClassName, | ||
onClick: startIconOnClick, | ||
|
@@ -67,9 +66,12 @@ const Input = forwardRef<HTMLInputElement, InputProps>( | |
...restOfEndIconProps | ||
} = endIconProps || {}; | ||
|
||
const handleOnChange = (event: React.ChangeEvent<HTMLInputElement>) => { | ||
onChange?.(event.target.value); | ||
}; | ||
const handleOnChange = useCallback( | ||
(event: React.ChangeEvent<HTMLInputElement>) => { | ||
onChange?.(event.target.value); | ||
}, | ||
[onChange], | ||
); | ||
|
||
isValid = isValid === undefined ? !errorMessage : isValid; | ||
|
||
|
@@ -116,7 +118,6 @@ const Input = forwardRef<HTMLInputElement, InputProps>( | |
<StyledInput | ||
as={renderAs} | ||
type={type} | ||
{...focusProps} | ||
{...inputProps} | ||
UNSAFE_height={UNSAFE_height} | ||
UNSAFE_width={UNSAFE_width} | ||
|
@@ -126,7 +127,6 @@ const Input = forwardRef<HTMLInputElement, InputProps>( | |
hasEndIcon={!!endIcon} | ||
hasStartIcon={!!startIcon} | ||
inputSize={size} | ||
isFocusVisible={isFocusVisible} | ||
onChange={handleOnChange} | ||
readOnly={isReadOnly} | ||
ref={inputRef} | ||
|
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.
is not what? Do we even need this test?
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.
This is what I posted in channel where the soft focus behaviour for search input is there