Closed borisghidaglia closed 1 month ago
The latest updates on your projects. Learn more about Vercel for Git βοΈ
Name | Status | Preview | Comments | Updated (UTC) |
---|---|---|---|---|
meet.hn | β Ready (Inspect) | Visit Preview | π¬ Add feedback | Sep 18, 2024 6:39pm |
Related to #6 from @reynaldichernando.
Thanks for your contribution! I was surprised by the fix your PR implemented. It seemed that something was off in a more general way. And indeed your PR made me realize that some inconsistencies were creating several problems. I did a more general cleanup which led to fixing the issue you raised as well as two others.
I let you review this (if you want) and tell me what you think :)
Hi @borisghidaglia, thanks for pinging me.
I like your approach for point 1 and 2 is by using the url
field instead of the value
field, it is a more clean approach and it works as expected.
For point 3 and 4, I see that your implementation is by adjusting the keys so that the defaultValue
is being updated when changing user. (ref: https://stackoverflow.com/questions/30146105/react-input-defaultvalue-doesnt-update-with-state)
This works, however, what happens is for every change (e.g., GitHub name) the input loses focus since it keeps re-rendering the component after key change. Here's a video of it happening: https://www.loom.com/share/9832dee50aca4598aeed8c1d77bffd7c?sid=f264a292-7ea7-4ebe-97ec-332d95f3281a.
The reason defaultValue
didn't change was because it is only an initial value. I think for point 3 and 4 we could try to use value
props instead of defaultValue
(along with adjustments for the onChange
event handler) to produce the same result, which is updating the values whenever the user changes.
Besides this, the fix for link prefix is working as expected. π
the input loses focus since it keeps re-rendering the component after key change
Oh yeah thanks you are right. I had forgotten about this behaviour even though I encountered it before π€¦π»: https://github.com/borisghidaglia/meet-hn/commit/6199ee6fa50225878ba9881de6a8811522b17cc8
I think for point 3 and 4 we could try to use value props instead of defaultValue
I wanted to avoid adding a state and make this a controlled input (what you suggest) but I don't see an alternative right now. So I'll give it a shot.
Pushed a WIP. Two things I didn't managed to do:
I wonder if we really have to switch to controlled inputs for all these...
I got tempted and worked on this today, but tomorrow I'll have to work on cities problems!
Hey @reynaldichernando, this should be fixed with the latest commit: https://github.com/borisghidaglia/meet-hn/commit/c2aa78b960027c26146c86fadc331efe4767bed5.
It includes a bunch of things that I didn't bother splitting into individual commits πΆβπ«οΈ
Would you like me to include you in the readme as a thank you?
Hi again @borisghidaglia! and wow, thatβs a big commit π .
Would you like me to include you in the readme as a thank you?
Thank you for offering, I would very much appreciate that. π
and wow, thatβs a big commit π
Yes it is π I actually made a dozen of WIP over a few days and squashed them all into one as they were not meaningful on their own.
Thank you for offering, I would very much appreciate that. π
Great! Will do, then!