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

Merge @nextcloud/vue-richtext into @nextcloud/vue #3841

Merged
merged 4 commits into from
Mar 2, 2023

Conversation

raimund-schluessler
Copy link
Contributor

@raimund-schluessler raimund-schluessler commented Mar 1, 2023

This PR merges @nextcloud/vue-richtext into the repository here. I mainly copied the code from vue-richtext, adjusted the import paths, fixed lint errors and removed the dependency to vue-richtext.

The NcRichText example in the docs seems to work fine: https://deploy-preview-3841--nextcloud-vue-components.netlify.app/#/Components/NcRichText?id=ncrichtext-1
But a deeper check is still necessary, since I don't know the internal of vue-richtext and which exports are necessary. We also need to figure out how to handle the richtext.scss file.
Anyway, it would be best if you could check whether @nextcloud/vue more or less works as a drop-in-replacement for @nextcloud/vue-richtext now.

I still have to fix the jsdoc lint warnings, though.

Closes #3812.

@raimund-schluessler raimund-schluessler added this to the 7.8.0 milestone Mar 1, 2023
@raimund-schluessler raimund-schluessler added 2. developing Work in progress component Component discussion and/or suggestion dependencies Pull requests that update a dependency file feature: rich-contenteditable Related to the rich-contenteditable components technical debt feature: richtext Related to the richtext component labels Mar 1, 2023
Signed-off-by: Raimund Schlüßler <raimund.schluessler@mailbox.org>
Signed-off-by: Raimund Schlüßler <raimund.schluessler@mailbox.org>
Copy link
Contributor

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

Wow that was fast! Thank you very much. ❤️

I tried it with the latest spreed master. The link picker in NcRichContenteditable works like a charm.

We need to expose the searchProvider function (in src/components/NcRichText/NcReferencePicker/providerHelper.js) because it is used in Text (which has its own implementation using Tribute).
It is exposed in src/components/NcRichText/index.js.

I'll make some more tests with Text soon.

@raimund-schluessler

This comment was marked as resolved.

@julien-nc
Copy link
Contributor

Ok, test results are all green:

Spreed

  • import and use NcRichText (import NcRichText from '@nextcloud/vue/dist/Components/NcRichText.js)
  • import and use NcRichContendeditable and check the link picker works fine with all kinds of providers
  • uninstall @nextcloud/vue-richtext and remove all style import

Text

  • import/use searchProvider and getLinkWithPicker to trigger the link picker
  • import and use NcReferenceList (import { NcReferenceList } from '@nextcloud/vue/dist/Components/NcRichText.js)
  • uninstall @nextcloud/vue-richtext and remove all style import

All good from my pov. ✔️ ❤️

I'll prepare the PRs to bring this in spreed and text to the next Vue lib release.

@raimund-schluessler

This comment was marked as resolved.

@julien-nc julien-nc self-requested a review March 1, 2023 15:00
@raimund-schluessler
Copy link
Contributor Author

Ok, test results are all green:

Great to hear! Thanks a lot for checking.

@raimund-schluessler raimund-schluessler marked this pull request as ready for review March 1, 2023 15:01
@raimund-schluessler raimund-schluessler added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Mar 1, 2023
@raimund-schluessler raimund-schluessler changed the title Merge nextcloud/vue-richtext into nextcloud/vue Merge @nextcloud/vue-richtext into @nextcloud/vue Mar 1, 2023
Copy link
Contributor

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

Successfully tested in spreed and text

  • link picker stuff ✔️
  • reference widget stuff ✔️
  • NcRichText ✔️

Signed-off-by: Raimund Schlüßler <raimund.schluessler@mailbox.org>
@raimund-schluessler
Copy link
Contributor Author

Let me also add the richtext.spec.js test from nextcloud/vue-richtext. Then it should be good.

Signed-off-by: Raimund Schlüßler <raimund.schluessler@mailbox.org>
Copy link
Contributor

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

Code looks good to me 👌

I didn't test myself, but apparently @julien-nc did so ☺️

Great to get @nextcloud/vue-richtext incorporated into @nextcloud/vue

@mejo-
Copy link
Contributor

mejo- commented Mar 1, 2023

Tested now as well with Text and works like a charm 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews component Component discussion and/or suggestion dependencies Pull requests that update a dependency file feature: rich-contenteditable Related to the rich-contenteditable components feature: richtext Related to the richtext component technical debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Circular dependency between @nextcloud/vue and @nextcloud/vue-richtext
4 participants