Skip to content

Conversation

@juliusknorr
Copy link
Member

@juliusknorr juliusknorr commented Dec 29, 2022

📝 Summary

Very early draft for implementing a frontend API so that the text editor can be used by other apps.

There is a small demo page temporarily available at /apps/text/ that shows both usages as a file editor (e.g. for collectives) and as a editor based on providing markdown content (e.g. for deck).

For now I've introduced a new MarkdownContentEditor component that is being used for editing text that is not sourced from files. This makes use of most components that are already used in the Editor component (which is currently the one for files).

Can be tested with

🚧 TODO

  • Proof of concept for exposing the editor through a plain JS API
  • Implement MarkdownFileEditor as wrapper around Editor and RichTextReader
  • Disable non-functional parts
    • Attachment upload
    • File picker (still to test - should work in theory)
    • Mentions
  • Settle on API
  • Check on all requirements of external apps
  • Discuss on potential input/output formats (e.g. tables currently uses html) -> We should move all to markdown to keep things simple
  • Cleanup before merge
    • Remove demo page commit (62ec547)
  • Document limitations of the content only mode

Follow up

  • Probably wrap API to typed package
  • Things that collectives triggers
    • Save through syncService.save()
    • focusEditor

🏁 Checklist

  • Code is properly formatted (npm run lint / npm run stylelint / composer run cs:check)
  • Sign-off message is added to all commits
  • Tests (unit, integration and/or end-to-end) passing and the changes are covered with tests
  • Documentation (README or documentation) has been updated or is not required

@cypress
Copy link

cypress bot commented Dec 30, 2022

1 flaky tests on run #8418 ↗︎

0 132 0 0 Flakiness 1

Details:

Editor frontend API
Project: Text Commit: 49a898fc1c
Status: Passed Duration: 05:43 💡
Started: Feb 1, 2023 8:35 PM Ended: Feb 1, 2023 8:41 PM
Flakiness  cypress/e2e/share.spec.js • 1 flaky test

View Output Video

Test
Open test.md in viewer > Share a file with download disabled shows an error Screenshot

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@juliusknorr juliusknorr force-pushed the enh/editor-api branch 2 times, most recently from 9943ce5 to 876d4aa Compare December 30, 2022 11:26
@juliusknorr
Copy link
Member Author

@mejo- @max-nextcloud Pinging for some early feedback on the general concept. I refactored quite a bit in terms of how the tiptap plugins are being gathered, but this should not have any breaking change for now in terms of the existing file editor and the components exposed to the npm packages.

Copy link
Collaborator

@max-nextcloud max-nextcloud left a comment

Choose a reason for hiding this comment

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

Looks like a great start to me. 👍

At some point we might want a switch in collectives to toggle editing / read only. But from my point of view that does not have to land in a first iteration.

I really like the cleanup of the EditorFactory, moving the tiptap extensions into RichText.

@susnux
Copy link
Contributor

susnux commented Dec 31, 2022

So if I understand it correctly this provides editor related code under OCA.... global variable?
What is the benefit over the already existing @nextcloud/text package?

Would it not make more sense to use @nextcloud/text also for the text app and provide it through federated modules (supported by webpack and by vite) so other apps can use without needing to provide it?

@mejo- mejo- force-pushed the enh/editor-api branch 2 times, most recently from 7947962 to f6d2fea Compare January 19, 2023 09:34
mejo- added a commit to nextcloud/collectives that referenced this pull request Jan 19, 2023
Preparations for using the new editor API from Text, see
nextcloud/text#3615

Signed-off-by: Jonas <[email protected]>
mejo- added a commit to nextcloud/collectives that referenced this pull request Jan 19, 2023
Preparations for using the new editor API from Text, see
nextcloud/text#3615

Signed-off-by: Jonas <[email protected]>
mejo- added a commit to nextcloud/collectives that referenced this pull request Jan 25, 2023
Preparations for using the new editor API from Text, see
nextcloud/text#3615

Signed-off-by: Jonas <[email protected]>
@juliusknorr
Copy link
Member Author

juliusknorr commented Jan 25, 2023

Before the merge:

  • Remove demo page
  • Disable mentions if no callback is provided

mejo- added a commit to nextcloud/collectives that referenced this pull request Jan 25, 2023
Preparations for using the new editor API from Text, see
nextcloud/text#3615

Signed-off-by: Jonas <[email protected]>
@mejo-
Copy link
Member

mejo- commented Jan 25, 2023

So if I understand it correctly this provides editor related code under OCA.... global variable? What is the benefit over the already existing @nextcloud/text package?

Would it not make more sense to use @nextcloud/text also for the text app and provide it through federated modules (supported by webpack and by vite) so other apps can use without needing to provide it?

Sorry for the late reply @susnux. We discussed the pros and cons of different ways to provide Nextcloud Text as a global editor for different apps back and forth several times.

The main advantage of the approach with providing the editor via OCA.Text global is that there will be only one Text version on a particular instance. Using a NPM module sooner or later would result in different Text versions with an incompatible featureset being used.

@juliusknorr
Copy link
Member Author

Will rebase on top of #3700 since that refactors the mentions a bit

@juliusknorr juliusknorr force-pushed the enh/editor-api branch 2 times, most recently from 37900dc to 1a1e304 Compare January 30, 2023 13:43
@juliusknorr juliusknorr changed the base branch from main to enh/link-picker January 30, 2023 13:43
@juliusknorr juliusknorr marked this pull request as ready for review January 30, 2023 13:46
Copy link
Member

@mejo- mejo- left a comment

Choose a reason for hiding this comment

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

Huge thanks to @juliushaertl for all the work here ❤️

@juliusknorr
Copy link
Member Author

/compile

Copy link
Member Author

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

Demo still needs to be dropped

juliusknorr and others added 16 commits February 1, 2023 21:27
Signed-off-by: Julius Härtl <[email protected]>
Signed-off-by: nextcloud-command <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants