Skip to content
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 some issues in clear button for input fields #1512

Merged
merged 4 commits into from
Nov 2, 2024

Conversation

riccardobl
Copy link
Member

@riccardobl riccardobl commented Oct 24, 2024

This PR does two changes:

  • it hides the clear button if the field is readonly
  • changes the way the clear button render is toggled by making it effect/state based

the latter is to workaround the issue discovered here: #1479

In the way sending wallets are implemented, they are initialized at render time pulling data from localStorage, my current theory is that since this is skipped in SSR, there is the possibility of having the field resulting empty on the server and filled on the client and since the clear button is toggled conditionally based on this at render time, it can result in an hydration error.
By moving the toggler client side we avoid that.

@riccardobl riccardobl requested a review from ekzyis October 24, 2024 14:36
Copy link
Member

@ekzyis ekzyis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since the clear button is toggled conditionally based on this at render time, it can result in an hydration error.

Ahh, that makes sense, thanks for finding this out.

I think using useIsClient instead of useEffect makes more sense here. You're using useEffect to compute a value that you could compute during render so it looks like a typical unnecessary usage of useEffect. The intention is to hide the button during SSR which matches closer what useIsClient is for.

diff --git a/components/form.js b/components/form.js
index 26a956af..11052c42 100644
--- a/components/form.js
+++ b/components/form.js
@@ -33,6 +33,7 @@ import EyeClose from '@/svgs/eye-close-line.svg'
 import Info from './info'
 import { useMe } from './me'
 import classNames from 'classnames'
+import { useIsClient } from './use-client'

 export class SessionRequiredError extends Error {
   constructor () {
@@ -455,6 +456,7 @@ function InputInner ({
   const [field, meta, helpers] = noForm ? [{}, {}, {}] : useField(props)
   const formik = noForm ? null : useFormikContext()
   const storageKeyPrefix = useContext(StorageKeyPrefixContext)
+  const isClient = useIsClient()

   const storageKey = storageKeyPrefix ? storageKeyPrefix + '-' + props.name : undefined

@@ -538,7 +540,7 @@ function InputInner ({
           isInvalid={invalid}
           isValid={showValid && meta.initialValue !== meta.value && meta.touched && !meta.error}
         />
-        {(clear && field.value) &&
+        {(isClient && clear && field.value && !props.readOnly) &&
           <Button
             variant={null}
             onClick={(e) => {

These hydration bugs because of client vs server state are pretty annoying. I am not sure useIsClient is the best solution but I think it's the best idiomatic one we've found so far.

you probably didn't know this hook existed but now you do haha

@riccardobl riccardobl requested a review from ekzyis October 24, 2024 18:06
@ekzyis ekzyis mentioned this pull request Oct 24, 2024
@huumn huumn merged commit a253e42 into stackernews:master Nov 2, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants