-
Notifications
You must be signed in to change notification settings - Fork 109
chore(migrate): useEditorMixin to useEditor composable #7313
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
Merged
Merged
Changes from all commits
Commits
Show all changes
23 commits
Select commit
Hold shift + click to select a range
881ce5b
chore(migrate): useEditorMixin to useEditor composable
max-nextcloud 8d998f0
chore(migrate): setContent mixin...
max-nextcloud 13a5ce1
chore(migrate): to useEditorFlags composable
max-nextcloud d958a4d
chore(cleanup): fix small review remarks
max-nextcloud b0790dc
chore(migrate): use.find instead of deprecated .contains
max-nextcloud 8b4e635
enh(editor): store session in separate extension
max-nextcloud 9d3cc87
chore(refactor): configure mention in rich text extension
max-nextcloud c4f863f
chore(types): collaborationCursor extension to typescript
max-nextcloud 354ad41
chore(simplify): rely on updateUser command
max-nextcloud 022e483
chore(simplify): replace computed fileExtension with temp
max-nextcloud 0809c8e
enh(code): start to load syntax highlighting during setup
max-nextcloud b326875
chore(refactor): load editor in mounted
max-nextcloud 3e5cfd8
chore(refactor): create editor in created instead of mounted
max-nextcloud ab26032
chore(refactor): create ydoc in setup
max-nextcloud 2831462
fix(character-count): always provide the current editors doc
max-nextcloud d7df4c0
fix(character-count): use the NcActionTexts name prop
max-nextcloud e24d90a
fix(loading): only show main container when content loaded
max-nextcloud 0d5f4f7
fix(mention): use shallowRef for connection
max-nextcloud 057a682
fix(load): create initial YjsState with dir
max-nextcloud fae62fc
chore(refactor): extract useEditor into its own file
max-nextcloud 17f0d62
chore(refactor): extract useEditorFlags into its own file
max-nextcloud 90b1d84
test(RichTextReader): basic test
max-nextcloud fabc79e
test(RichTextReader): update content
max-nextcloud File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You now check in many places whether
editorexists. This means that we cannot be sure thatuseEditorreturns an editor object and always have to deal witheditorbeingundefined? It looks a bit like an anti-pattern to me 🤔If I get it right, then
editorcan be set by all the base components that callprovideEditor()insetup()and then setthis.editorincreated()(i.e.BaseReader,MarkdownContentEditorandEditor). Since all consumers ofuseEditor()should be descendants (i.e child/granchild components) of the base editor components,editorshould always already be set when used there, no?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two things that come into play here right now:
injectreturns an optional value - i.e. can always be undefined. It's possible to cast it to always be defined (see last example in the docs for typing provide/inject ). That implies that the component will behave in unpredicted ways - most likely throwing exceptions when being used withoutprovideing a value.editorstarts as undefined in the providing component such asEditor.vue. We only initialize it once we loaded the content.2 is maybe artifact from how the content used to be loaded. I think we could be able to create the editor itself in the components setup routine these days. I'm a bit hesitant to changing the loading order right now as it feels fragile - but initializing the editor in the setup function rather than in loaded seems like it might even be a win for load times.
Thus far we hid these issues by only rendering subcomponents once the
$editorwas loaded. Whenever we removed thev-if='hasEditor'somewhere we'd get a lot of errors aboutthis.$editorbeing undefined where it was not expected.In addition the file that is rendered in an editor component never changes. If we switch between files in collective I believe the Editor component is unmounted and mounted again with a new value for the file related props. This is not how other components work, where one can just update a prop. This also means that for example the entire Menubar is recreated - with basically the same content. I was thinking that we might be able to keep the Editor component and change the props instead at some point. So far my assumption was that this would lead to
editorbeingundefinedwhile the new file is loaded. Theydocthat tracks the editing would also need to be replaced and it's part of thecollaborationplugin config. But maybe we can even keep the editor around and "just" replace its content, the ydoc and the file related parts of the sync.Anyway... I will take a look and see if I can initialize the
editorin the setup function already. Then we might be able to do away with the?everywhere.