fix: Address different behavior between `getHTML` and `generateHTML` #5366

baseballyama · 2024-07-18T09:34:43Z

Changes Overview

Using generateHTML function from @tiptap/html package generates HTML like <li data-checked data-type="taskItem">.
However, in other parts of the code, the expected HTML is <li data-checked="true" data-type="taskItem">.
Due to this behavior, the selection state is lost when the HTML obtained using the generateHTML function is rendered again.

Implementation Approach

The value set for the data-checked attribute can be either true or undefined.

Testing Done

I manually tested both cases, using generateHTML and not using the generateHTML function, and confirmed that everything works correctly.

Verification Steps

You can check out this repository to see how it works

https://github.com/baseballyama/tiptap-tasklist-bug-repl

Additional Notes

N/A

Checklist

I have created a changeset for this PR if necessary.
My changes do not break the library.
I have added tests where applicable.
I have followed the project guidelines.
I have fixed any lint issues.

Related Issues

N/A

changeset-bot · 2024-07-18T09:34:46Z

🦋 Changeset detected

Latest commit: c734ef0

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

This PR includes changesets to release 54 packages

Name	Type
@tiptap/extension-task-item	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-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/starter-kit	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

netlify · 2024-07-18T09:34:58Z

✅ Deploy Preview for tiptap-embed ready!

Name	Link
🔨 Latest commit	`c734ef0`
🔍 Latest deploy log	https://app.netlify.com/sites/tiptap-embed/deploys/6698ee64d6adf50008b80ad0
😎 Deploy Preview	https://deploy-preview-5366--tiptap-embed.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

nperez0111 · 2024-07-18T09:47:43Z

packages/extension-task-item/src/task-item.ts

@@ -67,7 +67,7 @@ export const TaskItem = Node.create<TaskItemOptions>({
        keepOnSplit: false,
        parseHTML: element => element.getAttribute('data-checked') === 'true',


I wonder if it is the parseHTML that should be more lenient instead I don't think that it needs to be checked against true, just that it actually exists right?

Yea so my testing shows that when data-checked is on the element (i.e. generateHTML outputted <li data-checked>...) that it gives an empty string to say that it was set but has no vlaue.

As opposed to being completely empty which is just null.

So we should be more liberal in our parsing rather than more strict in our output and the result should be the same:

Please see my REPL repo. https://github.com/baseballyama/tiptap-tasklist-bug-repl
And actually there is data-checked='false' attribute.

nperez0111 · 2024-07-18T09:51:09Z

packages/extension-task-item/src/task-item.ts

@@ -67,7 +67,7 @@ export const TaskItem = Node.create<TaskItemOptions>({
        keepOnSplit: false,
        parseHTML: element => element.getAttribute('data-checked') === 'true',
        renderHTML: attributes => ({
-          'data-checked': attributes.checked,
+          'data-checked': attributes.checked ? 'true' : undefined,


Others may rely on the specific output that this function generates so I would rather not introduce this sort of a change to the output

Agreed: How about this? d938fa9

Oh but I think my repl project doesn't work with this change...

I believe this works as expected and reflects our consensus.
a5ebc62, c297eee

This reverts commit d938fa9.

nperez0111 · 2024-07-18T10:19:10Z

packages/extension-task-item/src/task-item.ts

+        parseHTML: element => {
+          const dataChecked = element.getAttribute('data-checked')
+
+          return dataChecked != null && dataChecked !== 'false'
+        },


I think I would do element.getAttribute('data-checked') === '' || element.getAttribute('data-checked') === 'true' To be explicit about what we accept

I don't have an opinion for this. updated: 0f4c85d

nperez0111

Looks good, thanks for the quick turn around

baseballyama · 2024-07-18T10:37:49Z

@nperez0111 I didn't pay attention to the branch and this PR got merged into the main branch. I think the correct branch is develop, right?😅 (I don't know why main was selected by default)

baseballyama · 2024-07-18T10:42:31Z

Thank you for the backmerge to develop @nperez0111!

nperez0111 · 2024-07-18T10:43:58Z

Yea, I saw that too. My mistake, it is on me to handle that.

I will wait to get the release out though, I've been releasing a lot lately and have had several complaints of how many releases have been going out lately.

…` instead of only when `<li data-checked="true"` (re-fix of #5366) (#5426) * fix * changeset

baseballyama requested review from bdbch and svenadlung as code owners July 18, 2024 09:34

baseballyama force-pushed the patch-1 branch from 41ee383 to 34df655 Compare July 18, 2024 09:35

fix: Correct selection state reflection with yjs rendering

53c66b9

baseballyama force-pushed the patch-1 branch from 34df655 to 53c66b9 Compare July 18, 2024 09:40

nperez0111 reviewed Jul 18, 2024

View reviewed changes

more flexible parser

d938fa9

baseballyama force-pushed the patch-1 branch from 72b887f to d938fa9 Compare July 18, 2024 10:00

baseballyama added 3 commits July 18, 2024 19:09

Revert "more flexible parser"

a5166ab

This reverts commit d938fa9.

more flexible parser (again)

a5ebc62

improve

c297eee

baseballyama requested a review from nperez0111 July 18, 2024 10:18

nperez0111 reviewed Jul 18, 2024

View reviewed changes

check explicit value

0f4c85d

baseballyama requested a review from nperez0111 July 18, 2024 10:27

chore: changeset wording

c734ef0

nperez0111 approved these changes Jul 18, 2024

View reviewed changes

nperez0111 merged commit e6c759b into ueberdosis:main Jul 18, 2024
14 checks passed

baseballyama deleted the patch-1 branch July 18, 2024 10:33

baseballyama mentioned this pull request Jul 31, 2024

fix: allow task items to be parsed when only having <li data-checked instead of only when <li data-checked="true" (re-fix of #5366) #5426

Merged

5 tasks

bdbch pushed a commit that referenced this pull request Jul 31, 2024

fix: allow task items to be parsed when only having `<li data-checked…

6543f05

…` instead of only when `<li data-checked="true"` (re-fix of #5366) (#5426) * fix * changeset

This was referenced Jul 31, 2024

Publish a new pre-release version #5422

Closed

Publish a new pre-release version #5451

Merged

nperez0111 mentioned this pull request Aug 6, 2024

v2.5.9 #5452

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Address different behavior between `getHTML` and `generateHTML` #5366

fix: Address different behavior between `getHTML` and `generateHTML` #5366

baseballyama commented Jul 18, 2024

changeset-bot bot commented Jul 18, 2024 •

edited

Loading

netlify bot commented Jul 18, 2024 •

edited

Loading

nperez0111 Jul 18, 2024

nperez0111 Jul 18, 2024

baseballyama Jul 18, 2024

nperez0111 Jul 18, 2024

baseballyama Jul 18, 2024 •

edited

Loading

baseballyama Jul 18, 2024

baseballyama Jul 18, 2024 •

edited

Loading

nperez0111 Jul 18, 2024

baseballyama Jul 18, 2024 •

edited

Loading

nperez0111 left a comment

baseballyama commented Jul 18, 2024 •

edited

Loading

baseballyama commented Jul 18, 2024

nperez0111 commented Jul 18, 2024

		@@ -67,7 +67,7 @@ export const TaskItem = Node.create<TaskItemOptions>({
		keepOnSplit: false,
		parseHTML: element => element.getAttribute('data-checked') === 'true',

fix: Address different behavior between getHTML and generateHTML #5366

fix: Address different behavior between getHTML and generateHTML #5366

Conversation

baseballyama commented Jul 18, 2024

Changes Overview

Implementation Approach

Testing Done

Verification Steps

Additional Notes

Checklist

Related Issues

changeset-bot bot commented Jul 18, 2024 • edited Loading

🦋 Changeset detected

netlify bot commented Jul 18, 2024 • edited Loading

✅ Deploy Preview for tiptap-embed ready!

nperez0111 Jul 18, 2024

Choose a reason for hiding this comment

nperez0111 Jul 18, 2024

Choose a reason for hiding this comment

baseballyama Jul 18, 2024

Choose a reason for hiding this comment

nperez0111 Jul 18, 2024

Choose a reason for hiding this comment

baseballyama Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

baseballyama Jul 18, 2024

Choose a reason for hiding this comment

baseballyama Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

nperez0111 Jul 18, 2024

Choose a reason for hiding this comment

baseballyama Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

nperez0111 left a comment

Choose a reason for hiding this comment

baseballyama commented Jul 18, 2024 • edited Loading

baseballyama commented Jul 18, 2024

nperez0111 commented Jul 18, 2024

fix: Address different behavior between `getHTML` and `generateHTML` #5366

fix: Address different behavior between `getHTML` and `generateHTML` #5366

changeset-bot bot commented Jul 18, 2024 •

edited

Loading

netlify bot commented Jul 18, 2024 •

edited

Loading

baseballyama Jul 18, 2024 •

edited

Loading

baseballyama Jul 18, 2024 •

edited

Loading

baseballyama Jul 18, 2024 •

edited

Loading

baseballyama commented Jul 18, 2024 •

edited

Loading