Skip to content

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 <[email protected]>
@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.

4 participants