-
Notifications
You must be signed in to change notification settings - Fork 11
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
Edit In Place component #166
Conversation
Deploying compound-web with Cloudflare Pages
|
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.
Honestly, I don't see anything wrong with keeping showSaved
internal to the component. Controlled components are preferable to uncontrolled components when the parent might reasonably want to have access the child's state, but I don't think that applies to the "saved" text here - it just isn't useful to know about.
setTimeout(() => { | ||
setShowSaved(false); | ||
}, 2000); |
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 believe this will log a warning if the component is unmounted while the timer is still running
and move the url to the right place
Because the error might be a failure to commit the change rather than the input being invalid.
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 will be great to have a playwright snapshot with the save and cancel buttons
So we can have a screenshot of it in its focussed state
Done, added as its own playwright test as it relies on the component having focus. I'm surprised we've got this far without having real playwright test tbh. |
A component that allows a user to edit text in place with buttons that reveal while editing.