oqtane / oqtane.framework

CMS & Application Framework for Blazor & .NET MAUI
http://www.oqtane.org
MIT License
1.87k stars 541 forks source link

[ENH] RichTextEditor Provider Abstraction #4358

Closed sbwalker closed 3 months ago

sbwalker commented 3 months ago

Oqtane Info

Version - 5.2.0

Describe the enhancement

PR #4316 introduced an abstraction for RichTextEditors. Additional work is needed to complete this enhancement:

Goal

The RichTextEditor component is intended to be a replacement for the standard textarea HTML element. It should be very simple for developers to use the RichTextEditor component in place of the textarea element.

Previously, the RichTextEditor component was a wrapper around QuillJS. In the 5.2.0 release the RichTextEditor component will include an abstraction layer which allows it to support multiple RichTextEditorProviders. This provides the extensibility to replace the functionality with alternate implementations.

Example

<textarea @bind="@_content"></textarea>

can be replaced with

<RichTextEditor Content="@_content" @ref="@editor"></RichTextEditor> ... private RichTextEditor editor; ... _content = htmltext.Content; ... string content = await editor.GetHtml();

RichTextEditor Component Standard Parameters (ie. those supported by standard textarea element)

public string Content { get; set; } public string Placeholder { get; set; } public bool ReadOnly { get; set; };

RichTextEditor Component Optional Parameter

public string Provider { get; set; }

allows a specific RichTextEditorProvider to be set for a component instance

RichTextEditor Component Standard Public Method:

GetContent()

**note that there is no need for a GetHTML() method

Configuration

Each Provider will have its own unique configuration options. Based on the variability, these options cannot be expressed using static code - they need to be expressed dynamically at runtime. This will require configuration which is stored at the site level (ie. Site Settings). A RichTextEditorProvider should provide a UI for configuring its various options.

Options Specific to current QuillJS Editor:

public bool AllowFileManagement { get; set; } = true; public bool AllowRichText { get; set; } = true;
public bool AllowRawHtml { get; set; } = true;
public RenderFragment ToolbarContent { get; set; } public string Theme { get; set; } public string DebugLevel { get; set; }

** note that these options were formerly defined as static parameters of the RichTextEditor however based on the new provider abstraction they need to be removed (this will be a breaking behavioral change for existing components which reference them)

Configuration

it should be possible to specify the RichTextEditorProvider for each Site. This should be an option in Site Settings. The option would display the Name of the RichTextEditorProvider to the user, and save its Type to the database.

RichTextEditorProviders

QuillJS - the default RichTextEditorProvider provider using QuillJS (same functionality as current Oqtane version) TextArea - a simple alternative RichTextEditorProvider using a standard textarea element - proves that the abstraction works as expected

Migration/Upgrade

Any custom configuration which is specific to the existing QuillJS editor will not be migrated automatically - it will need to be configured manually per Site using the QuillJS editor provider's config UI.

Testing

There are many run-time scenarios which have caused issues for the QuilljS editor over time (ie. #4134) - we need to make sure they all continue to function properly within the new abstraction

iJungleboy commented 3 months ago

awesome - that was quick 🚀

barely talked about it three weeks ago 💖

zyhfish commented 3 months ago

Hi @sbwalker , if we remove below properties from the RichTextEditor component, this will be a break change that all the references need to remove setting code to these properties: public bool AllowFileManagement { get; set; } = true; public bool AllowRichText { get; set; } = true; public bool AllowRawHtml { get; set; } = true; public RenderFragment ToolbarContent { get; set; } public string Theme { get; set; } public string DebugLevel { get; set; }

maybe we can mark these properties as obsoleted with a warning that these properties won't take any effect and need to set up in site settings, does this make sense?

remove GetHtml method from RichTextEditor component will also be a break change, should we also mark it as obsoleted and use GetContent to replace it?

also the Content property will be confused with GetContent method as they seems need to return same value, but property doesn't support to call to async methods.

sbwalker commented 3 months ago

@zyhfish In my original post I mentioned that this would be a behavioral breaking change, as I thought the extra attributes simply would be ignored - however I just tested this and you are correct that this would actually throw an exception - so it would be an actual breaking change.

In regards to your suggestion to include the parameters in the RichTextEditor but mark them as Obsolete... unfortunately Blazor does not support Obsolete parameters - see https://github.com/dotnet/razor/issues/7657.

This leaves a few possible options:

The second option seems to be better. And the second option could even be enhanced to be a generic way to pass extra static parameters to a specific RichTextEditorProvider. For scenarios where a developer plans on using a specific RichTextEditorProvider in their module component (ie. by specifying the Provider parameter) it would make sense that they may want to specify behavior for that RichTextEditorProvider as well. The RichTextEditor could use the generic parameter to capture the parameters and pass them to the RichTextEditorProvider. In this case if values were specified they would override any Site Settings which may have been set for the RichTextEditorProvider for the site.

sbwalker commented 3 months ago

in regards to your other comments....

"remove GetHtml method from RichTextEditor component will also be a break change, should we also mark it as obsoleted and use GetContent to replace it?"

my apologies... I got methods switched up - the GetHtml method is the standard method used most often in the current RichTextEditor - so it needs to be retained. The current RichTextEditor also had a public GetContent() method which I do not think was ever used. It appears that it was removed from the new RichTextEditor in the Dev branch - but still exists within the QuillTextEditor - not sure why? For compatibility should the new RichTextEditor retain an Obsolete GetContent() method?

"also the Content property will be confused with GetContent method as they seems need to return same value, but property doesn't support to call to async methods."

so there will be a Content parameter for passing content to the RichTextEditor and a GetHtml() method for retrieving content from the RichTextEditor

sbwalker commented 3 months ago

One other remaining challenge for creating RichTextEditorProviders is how to deal with Resources. There are 2 types of Resources:

JavaScript

Currently Script Resources are declared in the RichTextEditor. As long as a the component which is using the RichTextEditor is Interactive, the resources will be loaded into the page. This is because the Script Resources are injected using the OnAfterRenderAsync method - which is only valid for Interactive components. If you tried to use the RichTextEditor in a Static component, the Script Resources would not be loaded.

The handling of Script Resources is problematic for the new approach using RichTextEditorProviders that are loaded dynamically at run-time - in the event that a RichTextEditorProvider is intended to be used in Static render mode.

CSS

Currently Stylesheet Resources cannot be declared in the RichTextEditor, as they will be ignored. This is because the SiteRouter is only able to load Resources from Module and Theme components - as these are the only components it knows about (ie. their types are stored in the database and utilized when rendering a page). As a result the Stylesheet Resources for the Quill editor are currently declared in the HtmlText module Edit.razor component (rather than in the RichTextEditor itself). This behavior is the same for both Interactive and Static render modes.

The handling of Stylesheet Resources is problematic for the new approach using RichTextEditorProviders that are loaded dynamically at run-time.

This will require some deeper investigation.

sbwalker commented 3 months ago

One possible solution is an enhancement to IModuleControl to define a property for AdditionalResources - which would list any other Types that the component relies upon which have Resources that need to be registered. The SiteRouter could use this property to do reflection and capture the additional Resources from those Types. For dynamic scenarios like RichTextEditorProviders, the static type is not known until run-time - so it would be useful to also allow AdditionalResources to support the specification of a database setting rather than a Type ie. SiteSetting:RichTextEditorProvider which the SiteRouter could then use to get the type name and get the resources. This enhancement would resolve some current issues related to ModuleSettings, ThemeSettings, ContainerSettings, etc... which are embedded within other module components, making it difficult/impossible to access their Resources. Not that this is just a theoretical concept at this pint and would require more research.

zyhfish commented 3 months ago

in regards to your other comments....

"remove GetHtml method from RichTextEditor component will also be a break change, should we also mark it as obsoleted and use GetContent to replace it?"

my apologies... I got methods switched up - the GetHtml method is the standard method used most often in the current RichTextEditor - so it needs to be retained. The current RichTextEditor also had a public GetContent() method which I do not think was ever used. It appears that it was removed from the new RichTextEditor in the Dev branch - but still exists within the QuillTextEditor - not sure why? For compatibility should the new RichTextEditor retain an Obsolete GetContent() method?

RichTextEditor doesn't have GetContent method before, it's a new add method in QuillJSTextEditor, so that RichTextEditor can call the method to get content from the text editor instances.

thabaum commented 3 months ago

it should be possible to specify the RichTextEditorProvider for each Site. This should be an option in Site Settings. The option would display the Name of the RichTextEditorProvider to the user, and save its Type to the database.

@zyhfish Hello, thank you for your contribution! Question: Can we do this per module basis in the event a site may use multiple editors (Module Settings)? Maybe even by permission role? I am guessing if we create the logic to handle it should?

sbwalker commented 3 months ago

@thabaum there will be a property named Provider which a module can use to specify a specific RichTextEditorProvider it wants to use. It will NOT be a Module Setting configurable via the UI. It will also NOT provide variability by user role. These features are not in scope for the current enhancement.

thabaum commented 3 months ago

@sbwalker a module developed independently outside the framework could provide this however? I am understanding it wont get implemented into the site settings itself.

Example is if you want some users to use the quill editor and others to use the radzen by role, or by the HTML module inserted on the page. A way to override site settings, site default or module, if the one gets removed it defaults to default sort of things

I am thinking the Oqtane HTML module or third party could showcase this capability down the road.

sbwalker commented 3 months ago

@thabaum I am not sure if this would be possible in a custom RichTextEditorProvider or not.... but to be clear, it is NOT one of the requirements for the current enhancement for the 5.2 release. We are trying to keep the scope very narrow and focused.