Skip to content

Upgrade/tiptap2 #1981

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.

Sign up for GitHub

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

Jump to bottom
Merged
max-nextcloud merged 7 commits into from
Feb 9, 2022
Merged

Upgrade/tiptap2 #1981

max-nextcloud merged 7 commits into from
Feb 9, 2022

Conversation

@azul
Copy link
Contributor

@azul azul commented Dec 2, 2021 •
edited by max-nextcloud
Loading

Summary

This is still work in progress.

  • update dependencies
  • update imports
  • migrate marks
  • migrate extensions
    • emoji picker
    • user colors
    • Keymap
    • collaboration
  • integrate ported collaboration extension
  • migrate nodes:
    • ordered list
    • unordered list
    • todo list
    • block quote
    • code block
    • image
    • trailling node
  • plain text editing
    • port plain text document node
    • line breaks
    • syntax highlight
  • menu bubble
    • review positioning and styling
  • Bring back missing ui items:
    • Emtpy text placeholder
    • Toolbar in workspace
  • scroll to inserted images
  • make the trashcan icon work again on images
  • PollingBackend:fetchSteps Network error when fetching steps, retry 1

@juliusknorr
Copy link
Member

juliusknorr commented Dec 3, 2021

FYI I added plain text editing / syntax highlight to the checklist as that is something that has likely changed but we might be able to tackle #1942 (comment) with that.

@juliusknorr juliusknorr added this to the Nextcloud 24 milestone Jan 11, 2022
@ya-d ya-d mentioned this pull request Jan 14, 2022
Closed
@azul azul marked this pull request as draft January 18, 2022 16:58
@azul azul changed the title WIP: Upgrade/tiptap2 Upgrade/tiptap2 Jan 18, 2022
@max-nextcloud max-nextcloud force-pushed the upgrade/tiptap2 branch 4 times, most recently from 8fa565c to e133d85 Compare February 7, 2022 09:47
@max-nextcloud max-nextcloud marked this pull request as ready for review February 7, 2022 10:34
@max-nextcloud max-nextcloud requested review from a team, juliusknorr and luka-nextcloud and removed request for a team February 7, 2022 10:35
@julien-nc
Copy link
Member

julien-nc commented Feb 7, 2022

I think the test failure (while testing images features) is caused by a session sync happening after the user has been deleted. It does not happen all the time.
My bad. I'm on it.

@julien-nc
Copy link
Member

julien-nc commented Feb 7, 2022

#2167 fixes the Cypress image test.

julien-nc
julien-nc approved these changes Feb 7, 2022
View reviewed changes
Copy link
Member

@julien-nc julien-nc left a comment

Choose a reason for hiding this comment

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

Tried all aspects of the description task list, everything works fine and looks the same as before.
I didn't deeply check the code but lgtm.

It's incredible we don't loose anything with this tiptap upgrade.
Impressive work!

@mejo-
Copy link
Member

mejo- commented Feb 7, 2022

The keyboard shortcuts for strikethrough changed upstream (Ctrl-Shift-X instead of Ctrl-D). We either have to overwrite it or (imho better) keep it the upstream way and update our documentation.

@max-nextcloud
Copy link
Collaborator

max-nextcloud commented Feb 7, 2022

I think the test failure (while testing images features) is caused by a session sync happening after the user has been deleted. It does not happen all the time. My bad. I'm on it.

Hehe... Actually this caught an issue in the error handling. Before we were catching errors in the PollingBackend like this:

axios.post(...)
  .then(handleResponse)
  .catch(handleErrors)

This caused exceptions in handleResponse to be caught by the catch that was meant for network issues.
I had changed this to:

axios.post(...)
  .catch(handleErrors)
  .then(handleResponse)

Now when errors happened they would be handled by handleErrors and then handleResponse was triggered with the return value handleErrors (undefined). So that caused the failing cypress test we saw.

So in the end i changed this to:

axios.post(...)
  .then(handleResponse, handleErrors)

This will either handle the response or the errors - but not both.

@mejo-
Copy link
Member

mejo- commented Feb 7, 2022

The keyboard shortcuts for strikethrough changed upstream (Ctrl-Shift-X instead of Ctrl-D). We either have to overwrite it or (imho better) keep it the upstream way and update our documentation.

Same is true for shortcuts for headings (Ctrl-Alt-[1-6] instead of Ctrl-Shift-[1-6]).

@max-nextcloud max-nextcloud force-pushed the upgrade/tiptap2 branch 2 times, most recently from ac5d187 to 84d2535 Compare February 7, 2022 14:00
@max-nextcloud
Copy link
Collaborator

max-nextcloud commented Feb 8, 2022

The keyboard shortcuts for strikethrough changed upstream (Ctrl-Shift-X instead of Ctrl-D). We either have to overwrite it or (imho better) keep it the upstream way and update our documentation.

Ctrl-D is creating a bookmark in firefox by default. I don't think we should overwrite that. I don't use it - but seems to be a very core browser functionality.

In libreoffice ctrl-D is double underline. Strike through has no keyboard shortcut by default.
So i'm tempted to either go with ctrl+shift+X or no keyboard short cut at all.

@mejo-
Copy link
Member

mejo- commented Feb 8, 2022

Ctrl-D is creating a bookmark in firefox by default. I don't think we should overwrite that. I don't use it - but seems to be a very core browser functionality.

In libreoffice ctrl-D is double underline. Strike through has no keyboard shortcut by default. So i'm tempted to either go with ctrl+shift+X or no keyboard short cut at all.

Thanks for further looking into this. I can imagine that's the reason why TipTap switched from Ctrl-D to Ctrl-Shift-X and I think that we should follow. So only the shortcut documentation in src/components/MenuBar.vue and src/components/HelpModal.vue needs to be updated.

azul and others added 7 commits February 9, 2022 09:42
@azul @max-nextcloud
upgrade: tiptap v2
edfbb70 
Migrate the entire editor to tiptap v2.

Some changes were introduces that go beyond just using the new tiptap API:

*Collaboration*

Port tiptap1 collaboration.
We still want to use our session and sync mechanism.

*Serialization*

Add Markdown extension to handle serialization.
Tiptap config extensions are not automatically added
to the prosemirror schema anymore.
The extension adds the `toMarkdown` config value to the prosemirror schema.

With the new naming scheme tiptap nodes for a number of elements
do not match the prosemirror names.
Camelcase the marks and nodes from `defaultMarkdownSerializer`
so they match the tiptap names.

*Menubar*

* Specify args for isActive function directly rather than setting a function.
* Make use of the editor instance inside the MenuBar component.
* Use the editor rather than slots for command, focused etc.
* disable icons based on editor.can
* Show menubar as long as submenus are open.
  When opening a submenu of the menubar
  keep track of the open menu even for the image and the remaining action menu.
  Also refocus the editor whenever a submenu is closed.

*MenuBubble*

Let tippy handle the positioning

Tippy is really good at positioning the menu bubble.
Remove all our workarounds and let it do its thing.

In order for this to work the content of the MenuBubble
actually needs to live inside the tippy-content.

Tippy bases its calculations on the width of tippy-content.
So if we have the content hanging in a separate div
with absolute positioning
tippy-content will be 0x0 px and not represent the actual width
of the menu bubble.

*Upgrade image node and ImageView.*

Quite a bit of the syntax changed.
We now need a wrapping `<node-view-wrapper>` element.
Pretty good docs at https://tiptap.dev/guide/node-views/vue#render-a-vue-component

We also need to handle the async action.
It will run the action on it's own.
So in `clickIcon()` we need to test if the action returned anything.

Tiptap v1 had inline images.
v2 keeps them outside of paragraphs by default.
Configure Image node to use inline images as markdownit creates inline images right now.

*Trailing Node*

Tiptap v2 does not ship the trailing node extension anymore.
Included the one from the demos and turned it from typescript into javascript.

*Tests*

In order to isolate some problems tests were added.
The tests in Undeline.spec.js were green right from the beginning.
They are not related to the fix and only helped isolate the problem.

Also introduced a cypress test for Lists
that tests the editor
without rendering the page and logging in.
It is very fast and fairly easy to write.

*Refactorings*

* Split marks into separate files.

Signed-off-by: Max <[email protected]>
@azul @max-nextcloud
upgrade: syntax highlighting with tiptap2
f611a0f 
Signed-off-by: Max <[email protected]>
@max-nextcloud
fix: scroll image into view once it has been loaded
a4ab006 
Signed-off-by: Max <[email protected]>
@max-nextcloud
fix: use same lowlight version as tiptap
8f64aaa 
Lowlight 2.4.1 was causing failures with jest.
Jest was unhappy with it being an ESM module:
https://github.com/nextcloud/text/runs/5089748945

Do not hand around the lowlight global between EditorFactory and EditorWrapper.
Tiptaps CodeBlockLowLight does not need it anyway.

Signed-off-by: Max <[email protected]>
@max-nextcloud
fix: use tiptap v2 prop deleteNode to delete image
c1af1a3 
Signed-off-by: Max <[email protected]>
@max-nextcloud
fix: use tiptap.commands to focus
95c706d 
Either handle network errors or the response.

There are three ways to handle errors and responses with axios:
```
axios.post(...).then(...).catch(...)
axios.post(...).catch(...).then(...)
axios.post(...).then(onResponse, onErr)
```

Use the last one.
First one will catch errors triggered by the emits in the happy path
with the network error handling.
Second one will handle the response,
even if errors have been caught.

Signed-off-by: Max <[email protected]>
@max-nextcloud
fix: adjust keyboard shortcuts and help modal
87aa069 
Tiptap v2 changed some defaults for the keyboard shortcuts.

* use new default ctrl+shift+X for strike through.
  Old ctrl+d is used to create bookmarks in firefox.
* Keep old ctrl+shift+1 ... ctrl+shift+6 for headings.
  No reason to change as far as i can tell.
  Also more consistent with lists (ctrl+shift+7/8).
* Change ordered lists to ctrl+shift+7.
  New default from tiptap.
  This way it comes right after the headings.

Signed-off-by: Max <[email protected]>
juliusknorr
juliusknorr approved these changes Feb 9, 2022
View reviewed changes
Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

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

Awesome work 👍 Lets get this in

@max-nextcloud max-nextcloud merged commit 73b710e into master Feb 9, 2022
@max-nextcloud max-nextcloud deleted the upgrade/tiptap2 branch February 9, 2022 11:00
mejo-
mejo- reviewed Feb 9, 2022
View reviewed changes
src/EditorFactory.js
const modules = {}
for (let i = 0; i < languages.length; i++) {
const list = listLanguages()
console.info(list)
Copy link
Member

@mejo- mejo- Feb 7, 2022

Choose a reason for hiding this comment

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

This looks like a debugging leftover ;)

@juliusknorr juliusknorr mentioned this pull request Feb 14, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mejo- mejo- mejo- left review comments

@juliusknorr juliusknorr juliusknorr approved these changes

@julien-nc julien-nc julien-nc approved these changes

@luka-nextcloud luka-nextcloud Awaiting requested review from luka-nextcloud luka-nextcloud was automatically assigned from nextcloud/office

Assignees

No one assigned

Labels

2. developing technical debt

Projects

None yet

Milestone

Nextcloud 24

Development

Successfully merging this pull request may close these issues.

Migrate to tiptap v2

6 participants

@azul @juliusknorr @julien-nc @mejo- @max-nextcloud