-
Notifications
You must be signed in to change notification settings - Fork 767
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
Allow the <LinkFloatingToolbar /> component to be submitted with the 'enter' key when the <PlateEditor /> is contained within a <form></form> #3133
Conversation
The current floating link element exposes no button to trigger the insertion and relies on the user pressing enter. This is not a problem in itself but, if the user is placing the WYSIWYG editor within a form pressing enter will trigger the parent form's onSubmit method and the link will not be inserted. As such I have wrapped the floating insert in an blank form element which takes the form context away from the parent and means submission is handled by plate opposed to the wrapper.
In order to prevent hydration issues I have removed the nested form an instead opted for a keyDown listener which will preventDefault if the key is enter
🦋 Changeset detectedLatest commit: 381535b The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
apps/www/src/registry/default/plate-ui/link-floating-toolbar.tsx
Outdated
Show resolved
Hide resolved
As requested by @zbeyens I have added a hook to address udecode#3130 and, hopefully, close the issue. The hook simply allows the user to pass `preventDefaultOnEnterKeydown` to the `<LinkFloatingToolbar/>` component. In doing so the user will not experience an issue I encountered whereby pressing enter prior to filling in both fields the overall form is submitted without the desired link element. The hook can also take a callback function `onKeyDownCaptureCallback` which will allow the user to be able to use this functionality and still be able to add their own logic to the event -- we don't want to prevent them from being able to implement default functionality of their own. I have added unit tests for the hook itself and it seems to be all good. Run the full monorepo test suite locally and, again, all good. Hopefully this closes the issue
Hey @zbeyens I have implemented the hook and a unit test :) Sorry for the delay mate been a hectic month at work and in my personal life! |
…that it can capture the event for either of the two inputs
@@ -16,6 +16,7 @@ import { | |||
useFloatingLinkInsert, | |||
useFloatingLinkInsertState, | |||
} from '@udecode/plate-link'; | |||
import { useFormInputProps } from '@udecode/plate-utils'; |
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.
import { useFormInputProps } from '@udecode/plate-utils'; | |
import { useFormInputProps } from '@udecode/plate-common'; |
preventDefaultOnEnterKeydown, | ||
onKeyDownCaptureCallback, |
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.
Props are not needed since you own the code
@@ -67,26 +74,24 @@ export function LinkFloatingToolbar({ state }: LinkFloatingToolbarProps) { | |||
editButtonProps, | |||
unlinkButtonProps, | |||
} = useFloatingLinkEdit(editState); | |||
const inputProps = useFormInputProps({ | |||
preventDefaultOnEnterKeydown, |
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.
preventDefaultOnEnterKeydown, | |
preventDefaultOnEnterKeydown: true, |
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.
Thanks for the PR! A few comments:
* @param e The original event | ||
* @returns nothing | ||
*/ | ||
onKeyDownCaptureCallback?: (e: React.KeyboardEvent<HTMLDivElement>) => void; |
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.
not needed, this can be done client side
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.
Not sure I fully understand this amend, sorry. I added that so that the user could use the pre-defined callback to preventDefault
and add in their own callback as well. Is your suggestion to either let the user use the predefined preventDefault
OR use their own callback?
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.
My comment was just on onKeyDownCaptureCallback
. preventDefaultOnEnterKeydown
is a good fit. Since the component is owned by the consumer, he can provide his own values inside these directly, instead of adding props.
@@ -67,26 +74,24 @@ export function LinkFloatingToolbar({ state }: LinkFloatingToolbarProps) { | |||
editButtonProps, | |||
unlinkButtonProps, | |||
} = useFloatingLinkEdit(editState); | |||
const inputProps = useFormInputProps({ | |||
preventDefaultOnEnterKeydown, | |||
onKeyDownCaptureCallback, |
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.
can be removed
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.
Again, not sure I fully understand the amend here, sorry. By removing this there would be no way of adding the onKeyDownCapture
callback to the main wrapping element of the <LinkFloatingToolbar />
as it does not allow for any props other than state
so, surely, the user would be required to wrap <LinkFloatingToolbar />
with a cosmetic <div>
on which they could then add the output of the useFormInputProps
to.
The amend above which says to add preventDefaultOnEnterKeydown: true,
on line 78 seems to contradict this also. Sorry mate, just not sure on what is requested/why it is requested.
|
||
if (hidden) return null; | ||
|
||
const input = ( | ||
<div className="flex w-[330px] flex-col"> | ||
<div className="flex w-[330px] flex-col" {...inputProps}> |
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.
@PaulSinghDev Sorry for the confusion, here is how a consumer could provide their own onKeyDownCapture
:
<div className="flex w-[330px] flex-col" {...inputProps} onKeyDownCapture={(e) => {
inputProps.onKeyDownCapture(e)
customOnKeyDownCapture(e)
...
Setting preventDefaultOnEnterKeydown: true
is just a default value the user can customize on his end
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.
@zbeyens No drama at all! I think I get you now, thanks for clarifying. With this, I'm guessing the suggestion is the user can alter the contents of <LinkFloatingToolbar />
themselves since we are following the ShadCN pattern of 'we give you the base, you change how you see fit'. I've just updated the code and the unit test to reflect these changes and will push now
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.
This is indeed the goal, maintaining props is otherwise a hell :)
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.
Nice one mate! I think I have addressed these issues in the latest commit. Sorry again I've been a bit dim ha
As the `<LinkFloatingToolbar />` can just be altered on the consumer's side we have removed the ability to pass a callback to also execute on the `onKeyDownCapture` event. By doing this we make the default behaviour to `preventDefault` and allow the consumer to make the decision as to whether, or not, to implement their own callback.
Please run again |
Done mate :) |
Thanks! Could you run again |
@zbeyens no probs at all, that has been done :) |
I encountered an issue whereby having the plate editor nested within a
<form></form>
lead to the<LinkFloatingToolbar />
triggering the form's submission method.This meant that the link element was never added into the editor's context and that the user couldn't actually add one in via the insert menu.
Solution
The change is a very basic one and is nothing major. I simply added an
onKeyDownCapture
event listener to the wrapping div element. Within the callback I simply check whether key pressed is theEnter
key and, if so, call thepreventDefault()
method exposed on the event.I opened an issue for this the day I noticed it and thought it would probbably help to just run through the tests etc and then make the PR to make everyone's lidfe easier.
The issues which this closes is: #3130