-
Notifications
You must be signed in to change notification settings - Fork 109
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
Keyboard support for palette entries #833
Conversation
f40f22e
to
406d992
Compare
406d992
to
9321c46
Compare
9321c46
to
b8b8733
Compare
b8b8733
to
152305d
Compare
The screencast is a bit misleading (focus border), seems like my screencast app is having some graphic troubles. It should work as expected in the demo. |
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.
works well for me
does it maybe make sense to add support in this PR to delete items from the form when focuses and the user presses delete?
For the delete via keyboard we had another PR that was blocked by a core issue: #438. I have to double-check whether this affects this PR as well. I will test this one with the Desktop Modeler. |
@pinussilvestrus the visual regression tests failed, were the changes intentional? |
Should not, I will have a look |
Works as expected 👍 I will handle the deletion shortcut separately still, as the PR is already open. Good candidate for spring cleaning as well. |
@volodymyr-melnykc @vsgoulart The failing visual regression test is because the icons are 1px moved compared to the previous version. I tried a few things to mitigate that, but it didn't work for all browsers reliably. I could take more time to fix this, but honestly, I'm not sure whether it's worth the time, as it doesn't look better or worse the other way around, in my opinion. Let me know what you think; if we are fine with the 1px change, I will update the screenshots to make the tests green. For comparison the new state (moving the diff-line from Playwright to the right). For me it even seems like the new state looks more centered. |
@pinussilvestrus It's ok for me! And yes, the new state (when the diff-line is moved to the right) looks more centered. |
89f01b4
to
5bd4ace
Compare
This makes the palette entries actual buttons so they can be controlled via keyboard. Additionally, this adds an
Enter
shortcut to the entries so one can add new form fields to the bottom of the form.Closes #536
form-js
element or visually changes an existing component.