-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Link Control: Go to edit #58862
Link Control: Go to edit #58862
Conversation
Warning: Type of PR label mismatch To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task. |
Size Change: +57 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
Thank you for trying this @scruffian - at first pass it is very surprising that focusing outside the popover makes the link disappear. I think we should explore:
|
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 know this isn't real code....BUT...I wanted to flag that I disagree with the idea of exposing internal state in the value object that is provided by onChange
.
...value, | ||
...internalControlValue, | ||
url: currentUrlInputValue, | ||
showEdit: false, |
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.
The value we return from LinkControl has a standardised shape and I don't think we should expose this type of internal rendering state on the value object.
Why? Because they are different concerns.
One is data.
One is rendering state.
If one day we decide on a different UX then this showEdit
will become a redundant property to support forever.
This doesn't seem like a very solid approach |
What?
This is just a prototype, not real code!
This is another attempt at #58279
Instead of saving the value when we select a link, we just update the component and display the save button. Then we commit the value when the save button is clicked.
Why?
This solves some accessibility concerns and potentially makes the flow smoother, although that's debatable.
How?
Adds a new piece of local component state - showEdit - which controls the visibility of the editing controls. There's probably a better way, this is just a prototype.
Testing Instructions
Screenshots or screencast
Feb-08-2024.22-01-37.mp4