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(core) Allow text style to be clearable on new lines #4151

Merged
merged 5 commits into from
Jul 7, 2023

Conversation

C-Hess
Copy link
Contributor

@C-Hess C-Hess commented Jun 24, 2023

Please describe your changes

When you have your cursor on a new line and some text style is applied (either from the Font Family or Color extensions), you cannot clear out the text styles when the cursor is on a new line.

It turns out there are two main factors that cause this behavior

Factor 1

We begin with the unset command of an extension that extends text-style. For example: font-family

https://github.com/ueberdosis/tiptap/blob/ccf05b04e3a2e2ee36c5bdac4d3b1a88bfe7a71d/packages/extension-font-family/src/font-family.ts#LL64C1-L67C17

Note that it first calls setMark then calls removeEmptyTextStyle. It doesn't matter what the two commands do. Really all that is causing this issue is the fact that we are calling another command after setMark within a chain()

Factor 2

Chained commands fail to persist stored marks. Look at this code snippet; it looks like in between chained commands, we are resetting the transaction's stored marks based on what is in state:

https://github.com/ueberdosis/tiptap/blob/ccf05b04e3a2e2ee36c5bdac4d3b1a88bfe7a71d/packages/core/src/CommandManager.ts#LL123C1-L125C6

As a result, the storedMarks fields cannot be updated if you run more than one chained command.

How did you accomplish your changes

Removing the logic that resets the transaction stored marks for chained commands resolves the issue. I really would need to know what prompted the change to begin with. All I have to go off of is this commit that I'm struggling to find an associated PR/issue for:
532eaa4

How have you tested your changes

Added automated tests around this behavior.

How can we verify your changes

Follow issue reproduction's steps in the color demo

Checklist

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

Related issues

fixes #3702

@netlify
Copy link

netlify bot commented Jun 24, 2023

Deploy Preview for tiptap-embed ready!

Name Link
🔨 Latest commit f1461eb
🔍 Latest deploy log https://app.netlify.com/sites/tiptap-embed/deploys/64973f3b4596f8000849712b
😎 Deploy Preview https://deploy-preview-4151--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 settings.

@C-Hess
Copy link
Contributor Author

C-Hess commented Jun 24, 2023

Hold: looks like I just figured out what that line does. Looks like without it, stored marks are lost when creating a list...

Looking at writing a test for that and trying to find a solution

@C-Hess
Copy link
Contributor Author

C-Hess commented Jun 24, 2023

Closing for now. Will need to think of a different approach

@C-Hess C-Hess closed this Jun 24, 2023
@C-Hess C-Hess reopened this Jun 24, 2023
@C-Hess C-Hess closed this Jun 24, 2023
@C-Hess C-Hess reopened this Jun 24, 2023
@C-Hess
Copy link
Contributor Author

C-Hess commented Jun 24, 2023

Okay false alarm, sorry for the mess here :)

Hold: looks like I just figured out what that line does. Looks like without it, stored marks are lost when creating a list...
Looking at writing a test for that and trying to find a solution

I was wrong about this, this behavior never existed. I can't find anything different before and after my changes

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.

I'm not sure if I can approve this. @svenadlung you want to take a look too? I could imagine this being at least a breaking change for some users who do work with styles being kept on new lines

packages/core/src/CommandManager.ts Show resolved Hide resolved
@bdbch bdbch requested a review from svenadlung June 25, 2023 01:13
@bdbch bdbch merged commit 7b4c792 into ueberdosis:develop Jul 7, 2023
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TextStyle cannot be cleared when cursor is on new line
2 participants