-
Notifications
You must be signed in to change notification settings - Fork 2.1k
feat: add focus event option for the checklist extension #8105
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
base: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
106ca67 to
bee20f2
Compare
etrepum
left a comment
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 would be good to add some tests for this feature, right now there's nothing that demonstrates that it works
|
This feature still doesn't have any tests, when you add some I will take another look |
I've added the test file. I took a little longer because i was trying to understand the standards for testing in Lexical, but i think i got it right. |
etrepum
left a comment
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.
In this case since we're dealing with focus, etc. and you're using mocks to change the environment's behavior it seems like a better approach would be to write an e2e test for this feature that uses a real browser. Mocking here makes it seem like maybe the tests aren't really doing anything meaningful.
|
E2E tests added. Tackling those conflicts now. |
9d51deb to
5f5cc9a
Compare
5f5cc9a to
8654206
Compare
b37b203 to
6ade0e2
Compare
…bles" for nested table pasting (facebook#8097)
6ade0e2 to
be46c84
Compare
Description
This PR adds an optional configuration parameter,
disableTakeFocusOnClick, toCheckListExtension. This allows users to enable or disable editor focus when clicking on checklist items, as described in the related issue:The changes are implemented in a way that does not break existing usages of
CheckListExtensionor theCheckListPluginReact component. The new parameter is optional and defaults to the current behavior.Note: In the file
LexicalCheckListPlugin.js.flow, I added what I believe is the correct change based on patterns observed in similar files. However, I did not deeply verify the purpose of all files in that directory, so this part should be reviewed.Test plan
I tested the behavior by running pnpm run start and verifying how checklist items behave when clicked, both with and without the
disableTakeFocusOnClickoption enabled.Steps:
packages/lexical-playground/src/appSettings.tsand setshouldDisableFocusOnClickChecklistto either false or true (default is false)pnpm run startI have not yet explicitly tested whether the mobile keyboard correctly dismisses when the option is set to true. However, in theory, this should work as expected, since the keyboard should only appear when the editor receives focus.
Additional Context
I noticed there is an existing open PR with the same goal as this one, but it was not completed by its author. For reference: