tiptap icon indicating copy to clipboard operation
tiptap copied to clipboard

fix: make blockquote shortcut work in starter-kit

Open Mathias-S opened this issue 2 years ago • 9 comments

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 #4994

Please describe your changes

Changed the order in which the bold/blockquote extensions are added in the starter-kit. This is because plugins added later will override plugins added earlier, because of f8efdf797a10a01235b75091729b15aca076e47a (#1547).

How did you accomplish your changes

Changed the order in which the bold/blockquote extensions 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/Default editor, which uses starter-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 that Mod-b (Control B / Cmd B) toggles Bold, as expected.

Remarks

It would be better if Mod-b didn't override Mod-shift-b regardless of plugin order, but it seems like this is handled by Prosemirror.

Checklist

  • [x] The changes are not breaking the editor
  • [ ] Added tests where possible
  • [x] Followed the guidelines
  • [x] Fixed linting issues

Related issues

  • Fixes #4994
  • Possibly caused by f8efdf797a10a01235b75091729b15aca076e47a (#1547)

Mathias-S avatar Mar 20 '24 09:03 Mathias-S

Deploy Preview for tiptap-embed ready!

Name Link
Latest commit 03e6f2375d0fc373698339d0f3fd676e59b0df85
Latest deploy log https://app.netlify.com/sites/tiptap-embed/deploys/66857f3dfa0452000881e7a0
Deploy Preview https://deploy-preview-4995--tiptap-embed.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify[bot] avatar Mar 20 '24 09:03 netlify[bot]

[!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 StarterKit code that would make you think that order is important for one to worry about when modifying that file.

IMO, a better solution is to increase the Blockquote extension priority to be higher than the Bold one. This is something we do ourselves in Typist (see here).

rfgamaral avatar Mar 20 '24 10:03 rfgamaral

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 priority, but I'm not sure it's the best way to solve it in starter-kit. Priority is explicitly modified based on execution order (see f8efdf797a10a01235b75091729b15aca076e47a), and if we were to increase Blockquote's priority directly in starter-kit, this might cause other users' custom extensions to no longer override blockquote from the starter-kit.

Mathias-S avatar Mar 20 '24 10:03 Mathias-S

and if we were to increase Blockquote's priority directly in starter-kit, this might cause other users' custom extensions to no longer override blockquote from the starter-kit.

I personally don't see that as a problem, as long as such a change is marked as breaking change.

Priority is explicitly modified based on execution order (see f8efdf7)

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 addProseMirrorPlugins.

rfgamaral avatar Mar 20 '24 11:03 rfgamaral

and if we were to increase Blockquote's priority directly in starter-kit, this might cause other users' custom extensions to no longer override blockquote from the starter-kit.

I personally don't see that as a problem, as long as such a change is marked as breaking change.

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 starter-kit fixes the bug without affecting anything else.

Priority is explicitly modified based on execution order (see f8efdf7)

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 addProseMirrorPlugins.

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.

Mathias-S avatar Mar 20 '24 12:03 Mathias-S

I agree, but I doubt TipTap is going to make a breaking release/major version bump to fix such a minor bug.

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.

rfgamaral avatar Mar 20 '24 12:03 rfgamaral

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?

bdbch avatar Mar 27 '24 18:03 bdbch

@svenadlung what are your thoughts?

bdbch avatar Mar 27 '24 18:03 bdbch

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?

The team has already made Link extension a higher priority to address some issues. I think making one other exception is not unreasonable.

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 StarterKit)

Nantris avatar Apr 12 '24 22:04 Nantris

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.

Mathias-S avatar Jul 01 '24 10:07 Mathias-S

Yea, I think that this is fine to merge in as is. I'll get it in

nperez0111 avatar Jul 03 '24 16:07 nperez0111

🦋 Changeset detected

Latest commit: 03e6f2375d0fc373698339d0f3fd676e59b0df85

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 54 packages
Name Type
@tiptap/starter-kit Patch
@tiptap/core Patch
@tiptap/extension-blockquote Patch
@tiptap/extension-bold Patch
@tiptap/extension-bubble-menu Patch
@tiptap/extension-bullet-list Patch
@tiptap/extension-character-count Patch
@tiptap/extension-code-block-lowlight Patch
@tiptap/extension-code-block Patch
@tiptap/extension-code Patch
@tiptap/extension-collaboration-cursor Patch
@tiptap/extension-collaboration Patch
@tiptap/extension-color Patch
@tiptap/extension-document Patch
@tiptap/extension-dropcursor Patch
@tiptap/extension-floating-menu Patch
@tiptap/extension-focus Patch
@tiptap/extension-font-family Patch
@tiptap/extension-gapcursor Patch
@tiptap/extension-hard-break Patch
@tiptap/extension-heading Patch
@tiptap/extension-highlight Patch
@tiptap/extension-history Patch
@tiptap/extension-horizontal-rule Patch
@tiptap/extension-image Patch
@tiptap/extension-italic Patch
@tiptap/extension-link Patch
@tiptap/extension-list-item Patch
@tiptap/extension-list-keymap Patch
@tiptap/extension-mention Patch
@tiptap/extension-ordered-list Patch
@tiptap/extension-paragraph Patch
@tiptap/extension-placeholder Patch
@tiptap/extension-strike Patch
@tiptap/extension-subscript Patch
@tiptap/extension-superscript Patch
@tiptap/extension-table-cell Patch
@tiptap/extension-table-header Patch
@tiptap/extension-table-row Patch
@tiptap/extension-table Patch
@tiptap/extension-task-item Patch
@tiptap/extension-task-list Patch
@tiptap/extension-text-align Patch
@tiptap/extension-text-style Patch
@tiptap/extension-text Patch
@tiptap/extension-typography Patch
@tiptap/extension-underline Patch
@tiptap/extension-youtube Patch
@tiptap/html Patch
@tiptap/pm Patch
@tiptap/react Patch
@tiptap/suggestion Patch
@tiptap/vue-2 Patch
@tiptap/vue-3 Patch

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

changeset-bot[bot] avatar Jul 03 '24 16:07 changeset-bot[bot]