fix: make blockquote shortcut work in starter-kit #4995
Conversation
✅ Deploy Preview for tiptap-embed ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Important I'm not a Tiptap maintainer, just someone that cares about it. Opinions are my own, and not the views of Tiptap maintainers. Relying on code order execution is generally not a good idea to fix an issue like this. There's nothing in the IMO, a better solution is to increase the |
|
I agree that I'm not a huge fan of simply changing the order, and it should at least have a comment in the code explaining it. In your own project, it makes a lot of sense to explicitly set |
I personally don't see that as a problem, as long as such a change is marked as breaking change.
I could be wrong, but I don't think that's related to extensions priorities, that's the ProseMirror plugin order, which is what is run when an extension makes use of |
I agree, but I doubt TipTap is going to make a breaking release/major version bump to fix such a minor bug. Changing the order within
That commit was made in response to #1547, which was about shortcut priority, so it's quite related. In fact, I think this bug is caused by that commit. |
I don't see why not. If it's a breaking change, no matter how small, it deserves its own major version. It's just a number. But again, I'm not a maintainer, I don't make decisions, just voicing my own opinion. I tend to prefer explicit code where comments explaining things are avoided, if possible. But that's just me. |
|
The change would be fine for me - yet it's something to be discussed as should all extension by themselves have the same priority as others or should they all come with their own "guessed" priorioty based on what could potentially overwrite them? |
|
@svenadlung what are your thoughts? |
The team has already made To change the priorities would probably fix #4006 (which I do not believe this PR as written would, since it might still affect anyone not using |
|
Anything that should be changed in order to get this merged? I agree with other commenters that finding a more reliable way of fixing this issue would be a good idea, but this specific change will simply fix the bug in starter-kit without really introducing any other possible issues, so it should be the lowest-risk way of fixing the bug and will help everyone just using the starter-kit. |
|
Yea, I think that this is fine to merge in as is. I'll get it in |
Because blockquote uses Mod-Shift-b, and bold uses Mod-b, the bold shortcut will override the blockquote shortcut if added to the extensions later. Fixes ueberdosis#4994
nperez0111
force-pushed
the
fix/starter-kit-blockquote-shortcut
branch
from
59c3ca1 to
dea216c
Compare
🦋 Changeset detectedLatest commit: 03e6f23 The changes in this PR will be included in the next version bump. This PR includes changesets to release 54 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 |
Because blockquote uses
Mod-Shift-b, and bold usesMod-b, the bold shortcut will override the blockquote shortcut if added to the extensions later.Fixes #4994
Please describe your changes
Changed the order in which the
bold/blockquoteextensions are added in the starter-kit. This is because plugins added later will override plugins added earlier, because of f8efdf7 (#1547).How did you accomplish your changes
Changed the order in which the
bold/blockquoteextensions are added in the starter-kit. I realise that this breaks the alphabetical ordering, but it's more important that the shortcuts work correctly.How have you tested your changes
Tested locally with the
Examples/Defaulteditor, which usesstarter-kit.How can we verify your changes
Load the Examples/Default example and check that
Mod-shift-b(Control Shift B/Cmd Shift B) toggles Blockquote, and thatMod-b(Control B/Cmd B) toggles Bold, as expected.Remarks
It would be better if
Mod-bdidn't overrideMod-shift-bregardless of plugin order, but it seems like this is handled by Prosemirror.Checklist
Related issues