Skip to content
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

Fix: configured/extended extensions should keep their original config #4191

Merged
merged 1 commit into from
Oct 10, 2023

Conversation

tleruitte
Copy link
Contributor

@tleruitte tleruitte commented Jul 7, 2023

Please describe your changes

Extensions, marks and nodes should keep their original config when they are extended or configured.

How did you accomplish your changes

n/a.

How have you tested your changes

Tested locally.

How can we verify your changes

import Code from '@tiptap/extension-code'
import Document from '@tiptap/extension-document'
import Paragraph from '@tiptap/extension-paragraph'
import Text from '@tiptap/extension-text'
import { EditorContent, useEditor } from '@tiptap/react'
import React from 'react'

export default () => {
  const editor = useEditor({
    extensions: [Document, Paragraph, Text, Code.configure({
      HTMLAttributes: {
        class: 'my-custom-class',
      },
    })],
    content: `
        <p>This isn’t code.</p>
        <p><code>This is code.</code></p>
      `,
  })

  if (!editor) {
    return null
  }

  return (
    <>
      <button
        onClick={() => editor.chain().focus().toggleCode().run()}
        className={editor.isActive('code') ? 'is-active' : ''}
      >
        toggleCode
      </button>
      <button
        onClick={() => editor.chain().focus().setCode().run()}
        disabled={editor.isActive('code')}
      >
        setCode
      </button>
      <button
        onClick={() => editor.chain().focus().unsetCode().run()}
        disabled={!editor.isActive('code')}
      >
        unsetCode
      </button>

      <EditorContent editor={editor} />
    </>
  )
}

Before this fix, the instance of Code doesn't have excludes, code or exitable set. This prevents the right arrow keyboard shortcut to exit the code mark for instance. After the fix, the config is correctly extended and the new instance of Code uses the correct values for excludes, code or exitable. Same goes for Extension and Node.

Remarks

This is especially noticeable when users use the StarterKit. StarterKit configures all extensions with undefined by default, which strips them all from their default config.

Checklist

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

Related issues

I haven't filed an issue.

@netlify
Copy link

netlify bot commented Jul 7, 2023

Deploy Preview for tiptap-embed ready!

Name Link
🔨 Latest commit b5ea1f2
🔍 Latest deploy log https://app.netlify.com/sites/tiptap-embed/deploys/64a979b1f6115d0008ebcdcf
😎 Deploy Preview https://deploy-preview-4191--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.

@tleruitte tleruitte changed the title Fix: configured extensions should keep their original config Fix: configured/extended extensions should keep their original config Jul 7, 2023
Copy link
Contributor

@bdbch bdbch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - this does not break any functionality while you tested it? @tleruitte

CC @svenadlung - you want to take another look?

@tleruitte
Copy link
Contributor Author

@bdbch Sorry I missed your comment. No, I didn't see any broken functionality with this fix!

@tleruitte
Copy link
Contributor Author

@bdbch @svenadlung Let me know if there is anything else I can do to get this merged :)

@rfgamaral
Copy link
Contributor

I'm wondering if this could be something released as v2.1.7 🤔😁

@tleruitte
Copy link
Contributor Author

I think this got fixed in 2.1.0 by another pull request. https://github.com/ueberdosis/tiptap/releases/tag/v2.1.0

@tleruitte
Copy link
Contributor Author

Never mind, just tested 2.1.11 and the bug is still present. #3822 only fix configure, but not extend. So I think we stil should merge this pull request. Let me know if/how I can help to get this merged.

@bdbch bdbch merged commit fda6310 into ueberdosis:develop Oct 10, 2023
5 checks passed
@bdbch
Copy link
Contributor

bdbch commented Oct 10, 2023

I merged it for now and will release a new RC version with this tomorrow. Would be nice if someone could check it out by then

@romansp
Copy link

romansp commented Oct 17, 2023

This appears as fixed in 2.2.0-rc.4.

Any idea why exitable is checked on config directly and not via getExtensionField helper? Though it doesn't seem to be necessary anymore because extension config is preserved now.

if (extension.type === 'mark' && extension.config.exitable) {
defaultBindings.ArrowRight = () => Mark.handleExit({ editor, mark: extension as Mark })
}

Also maybe exitable should be copied here?

const schema: MarkSpec = cleanUpSchemaItem({
...extraMarkFields,
inclusive: callOrReturn(
getExtensionField<MarkConfig['inclusive']>(extension, 'inclusive', context),
),
excludes: callOrReturn(
getExtensionField<MarkConfig['excludes']>(extension, 'excludes', context),
),
group: callOrReturn(getExtensionField<MarkConfig['group']>(extension, 'group', context)),
spanning: callOrReturn(
getExtensionField<MarkConfig['spanning']>(extension, 'spanning', context),
),
code: callOrReturn(getExtensionField<MarkConfig['code']>(extension, 'code', context)),
attrs: Object.fromEntries(
extensionAttributes.map(extensionAttribute => {
return [extensionAttribute.name, { default: extensionAttribute?.attribute?.default }]
}),
),
})

@Nantris
Copy link
Contributor

Nantris commented Nov 10, 2023

This commit causes #4617. See review comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

6 participants