-
Notifications
You must be signed in to change notification settings - Fork 64
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
🐛 TextField: dynamically update padding for icon #3380
Conversation
const [, updateState] = useState<object>({}) | ||
const forceUpdate = useCallback(() => updateState({}), []) | ||
//if rightAdornments are updated dynamically, we need to force re-render the component so the Input can set correct padding | ||
useEffect(() => { | ||
forceUpdate() | ||
}, [unit, inputIcon, forceUpdate]) |
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.
It would be nice if you could show me where we lose reactivity and why this is needed. I am hoping we can solve this without this complexity.
In my mind, the component should re-render when the prop changes, and then we recalculate the padding based on the new props.
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.
It does re-render when the prop changes and it does recalculate the padding. The problem is this all happens before the dom updates, so it is the old values. In Input
, this works correctly, but in TextField
, which wraps Input
we get the delayed dom update 🤷
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.
I understand now. The reason why it was not working as expected for the TextField
component was that we always passed the rightAdornments
property.
I added the hasRightAdornments
check so that we only pass it when unit
or inputIcon
are defined in the props.
…r inputIcon is defined
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.
I think this is a good solution, but you should probably test and confirm @oddvernes
* 🐛 TextField: dynamically update padding for icon * fix: only pass right adornment element to the component if the unit or inputIcon is defined * 📸 update snapshot --------- Co-authored-by: Torleif Halseth <halseth.torleif@gmail.com>
resolves #3354
I could not find any other way to make this work then forcing a re-render in
TextField
ifrightAdornments
changeUpdated the TextField "with icons" story with a button to toggle icon to demonstrate that it works