feat: add focus event option for the checklist extension#8105
feat: add focus event option for the checklist extension#8105rayterion wants to merge 13 commits intofacebook:mainfrom
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.
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.
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.
6ade0e2 to
be46c84
Compare
|
Looks like maybe there's an issue with firefox here since those e2e tests are failing |
I'll probably start fixing it monday. Once i finish fixing it, should i mention you somehow or maybe requesting a review...? |
|
I should get a notification when new commits are pushed |
…bles" for nested table pasting (facebook#8097)
ec28482 to
78c0c8c
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: