-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Add keyboardReturn submit button back to LinkControl #52620
Conversation
Size Change: +152 B (0%) Total Size: 1.51 MB
ℹ️ View Unchanged
|
We removed this when we added the |
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.
Let's update the unit tests that cover the creation of links to include the UI-based submit button as well as the keyboard ENTER
.
Can you help with those? 🙇♂️ |
Thanks for tying up all the loose ends here! 👍 👍 from me. |
Happy to help. I'll add it to our board. I might not get this is immediately though... |
@MaggieCabrera saw you're heading #50995; could you help get these tests updated per @getdave's comment: #52620 (review)? Should that be a separate PR/part of your existing effort — or in this one? Thanks! 🙇 |
Those are unit tests, I'm migrating the e2e tests from puppeteer to playwright, I think it's unrelated to this. I'll try to make time to have a look at this, see if I can help out. |
Writing tests now... |
0de48ef
to
b98f214
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Ok @richtabor this should be good to go. |
Mind looking at the tests one more time? |
Oh man. They broke 🤦 Re-running as didn't look like a legit failure. Unit tests are pretty reliable between local and CI. |
d237448
to
2cadaff
Compare
This failure is not consistent. Try running with
...sometimes it fails and sometimes not. |
Also fixes spinner on the internal url input
2cadaff
to
804bd3c
Compare
This is the test that is failing. Looks like the mocked function is inadvertently being called twice. |
The bug was because a call to This indicates a wider underlying but in the code whereby the calls are not reset if the component is unmounted. We'll need to address separately. However, it should not delay this PR. I fully expect my patch for the tests to pass and for us to merge this whilst I resolve the wider problem. |
I had to rerun the PHP tests, but everything passed fine after that. Nice work! |
What?
I couldn't find context why this was removed (if it was intentional), so here's a go at putting it back.
If we don't need the button itself, an alternate would be to employ the icon itself (which is what the block inserter search input does).
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast
Before:
CleanShot.2023-07-13.at.18.16.21.mp4
After:
CleanShot.2023-07-13.at.18.11.59.mp4