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

package: expose ImageResolver so it can be provided to RichTextReader #2514

Merged
merged 2 commits into from
Jul 4, 2022

Conversation

max-nextcloud
Copy link
Collaborator

  • Target version: master

Summary

In order to resolve images correctly in various contexts the RichTextReader needs an ImageResolver. The ImageResolver implements a resolve function that turns a src attribute into a full url that can be retrieved from the server.

Texts ImageResolver is quite powerful and flexible. So let's expose it so it can be used by other apps such as collectives.

@max-nextcloud
Copy link
Collaborator Author

max-nextcloud commented Jun 14, 2022

I was also considering some alternatives:

Providing the ImageResolver from within the RichTextReader

Pros:

  • More simple API, no changes needed on the collectives side.

Cons:

  • EditorWrapper provides ImageResolver and renders RichTextReader on conflicts. So it would be provided on two levels.
  • Less flexible - for example with respect to implementing a custom Image Resolver in index.html

Mixin for providing the ImageResolver

Pros:

  • less repetition, no need to export IMAGE_RESOLVER

Cons:

  • would add requirements such as this.currentDirectory to the component using the mixin.

@max-nextcloud
Copy link
Collaborator Author

@vinicius73 I'm curious to hear what you think about this approach. I'm quite unsure myself. I don't like to export such a complex API - but i have not been able to figure out something more simple.

@vinicius73
Copy link
Member

@vinicius73 I'm curious to hear what you think about this approach. I'm quite unsure myself. I don't like to export such a complex API - but i have not been able to figure out something more simple.

Since yesterday, I am thinking about this change.
Though, I share your fear... I did not think any technical issue related about this change.

If we provide a mixin, the main issue still the same:

How expose/provide: currentDirectory, shareToken, user and fileId to build ImageResolver

The pain point of this change is exposing the contract of ImageResolver and maybe turn hard to change it over the time.

Can we have problems if we pass it as props to RichTextReader?
For now is the only other way what I am able to think.

@max-nextcloud
Copy link
Collaborator Author

Can we have problems if we pass it as props to RichTextReader?
For now is the only other way what I am able to think.
I might give this a try. Seems simpler indeed as props are kind of the default api of components and we could also handle reactivity in one place.

The only downside i see is not being able to implement a different ImageResolver. I was thinking about providing one in index.html that allows using images with vite - but that's just a convenience thing for development.

@mejo- mejo- force-pushed the package/expose-image-resolver branch from ac2867d to fe8f436 Compare July 4, 2022 10:23
* Export the Image Resolver so it can be provided

Signed-off-by: Jonas <jonas@freesources.org>
@mejo- mejo- force-pushed the package/expose-image-resolver branch from fe8f436 to 3b72a0b Compare July 4, 2022 10:32
@mejo- mejo- marked this pull request as ready for review July 4, 2022 11:09
@mejo- mejo- merged commit ebc877f into master Jul 4, 2022
@delete-merged-branch delete-merged-branch bot deleted the package/expose-image-resolver branch July 4, 2022 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants