SimpleMachines / SMF

Simple Machines Forum — SMF in short — is free and open-source community forum software, delivering professional grade features in a package that allows you to set up your own online community within minutes!
https://www.simplemachines.org/
Other
595 stars 255 forks source link

Can't load more styles to sceditor #7681

Open DiegoAndresCortes opened 1 year ago

DiegoAndresCortes commented 1 year ago

Description

When toggling the source view in the sceditor, it is possible to load a provided file: https://www.sceditor.com/documentation/options/#style https://github.com/SimpleMachines/SMF/blob/5bc137dabe88aada50f2b450957f169e058f224a/Sources/Subs-Editor.php#L1912

We are loading jquery.sceditor.default.css and it's fine. However, this is a limitation for custom themes using multiple files (css variables, variants, dark mode, etc), it's currently not possible to load multiple files or pass data attributes to the iframe (so it can be targeted inside a single file).

sbulen commented 1 year ago

Did you have a possible solution in mind?

Note you can pass css variables to the editor inside the iframe. I do that in my themes, so the editor is updated dynamically. I've wondered why more folks don't use css variables in themes, given the flexibility.

DiegoAndresCortes commented 1 year ago

I only tested it with Namex theme since it's using dark mode and variants. I load the css variables using data-attributes in the html tag, but iframes can't be targeted using css only.

The styles option could be turned into an array so it allows to load multiple files, or pass the _$context['cssfiles'], and then we could move the content of jquery.sceditor.default.css to the index.css

Sesquipedalian commented 1 year ago

The styles option could be turned into an array so it allows to load multiple files, or pass the _$context['cssfiles'], and then we could move the content of jquery.sceditor.default.css to the index.css

I think the first of these two options is better. It would only require changes to files in Sources and is the more flexible solution. The second option would require changing the CSS files in the theme, and would do little to help mods (e.g. BBC mods) that might want to tweak the editor's CSS.

DiegoAndresCortes commented 1 year ago

That's fair about editing CSS files, though we've been doing it a bit in recent patches. In addition, what about the jquery.sceditor.theme.css file we already load for themes? With the second option it would be loaded with the rest of the files, and it's supposed to hold edits they might want to make to the editor (it's what we suggest authors to use when reviewing themes), and this particular issue is limiting the option of editing this bit inside the custom file.

DiegoAndresCortes commented 1 year ago

Actually, we didn't even consider that the first option is only useful and viable for mods hooking into _integrate_sceditoroptions. I was only thinking about my theme already using hooks, but the average theme can't/won't do that... They'd still be stuck if they are using variants or multiple styles.

sbulen commented 1 year ago

Question: Does sceditor accept an array?

https://github.com/samclarke/SCEditor/blob/0ee103397c5ed12a5a2cac8f397773a264226466/src/lib/defaultOptions.js#L32

Interesting that the comment there indicates that the style passed (jquery.sceditor.default.css) is specific to the wysiwyg elements only.

live627 commented 1 year ago

no

proof https://github.com/samclarke/SCEditor/blob/0ee103397c5ed12a5a2cac8f397773a264226466/src/lib/SCEditor.js#L583 template https://github.com/samclarke/SCEditor/blob/0ee103397c5ed12a5a2cac8f397773a264226466/src/lib/templates.js#L17

On Sat, Aug 5, 2023 at 1:32 PM sbulen @.***> wrote:

Question: Does sceditor accept an array?

https://github.com/samclarke/SCEditor/blob/0ee103397c5ed12a5a2cac8f397773a264226466/src/lib/defaultOptions.js#L32

Interesting that the comment there indicates that the style passed is specific to the wysiwyg elements only.

— Reply to this email directly, view it on GitHub https://github.com/SimpleMachines/SMF/issues/7681#issuecomment-1666599028, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADJNN3M7AQT2KNBBZV6R6TXT2UUJANCNFSM6AAAAAAUNFUVZI . You are receiving this because you are subscribed to this thread.Message ID: @.***>

DiegoAndresCortes commented 1 year ago

A 'solution' I found is to append things to the head inside the iframe

$(document).ready(function() {
    $('.sceditor-container iframe').each(function() {
        $(this).contents().find('head').append('..');
    });
}) 

I used _integrate_sceditoroptions though

Sesquipedalian commented 1 year ago

It wouldn't be ideal, but perhaps we could use SCEditor's css() method to add the extra CSS inline.

We'd have to get the contents of the CSS files in PHP and then feed them to the editor while setting it up. There would probably have to be some faffing around with our JavaScriptEscape() function and whatever else, but it might work.

sbulen commented 1 year ago

That style parameter is ONLY used for WYSIWYG mode tweaks...

Do we even really need to pass multiple files for that narrow a scope? It may be sufficient to be able to tell it which variant is being used, & fall back to jquery.sceditor.default.css if no variant applies.

The selected variant could help determine which .css to pass, e.g.:

The same approach could apply for the jquery.sceditor.theme.css... Have multiple copies, with the variant as part of the file name.

DiegoAndresCortes commented 1 year ago

Could make sense. Also, I suspect we created some confusion among users and ourselves. jquery.sceditor.theme.css was intended to overtake jquery.sceditor.css, but in reality it should overtake jquery.sceditor.default.css

sbulen commented 1 year ago

Yeah, I tried to sum up the current state here: https://www.simplemachines.org/community/index.php?msg=4157739

Actually, a few clarifications. Been a while since I looked into this... The fog is slowly lifting...

  • jquery.sceditor.theme.css covers the appearance of the sceditor area. From the rows of formatting buttons to the bottom of the textarea.
  • jquery.sceditor.default.css covers the textarea ONLY, in WYSIWYG mode ONLY.

So if you want a unique appearance of the editor, e.g., buttons & toolbars, etc., update jquery.sceditor.theme.css in your theme folder. It may help to start with a copy of jquery.sceditor.css from the default theme.

If you want to control WYSIWYG appearance within the editor textarea, update jquery.sceditor.default.css in your theme folder. It may help to start with a copy of jquery.sceditor.default.css from the default theme.

The editor is a 3rd party plugin we use. It accepts ONE custom css file as a parameter, specifically to support dynamic toggling of WYSIWYG mode.

That ONE file limitation for WYSIWYG mode makes certain tasks difficult, e.g., if you are trying to work on a theme with multiple color palette options such as light vs dark mode. If you need to do that, you need to do some work via code & css vars to work around those limitations (e.g., like I did with my themes).

Hope this helps. Clarifications & corrections welcome, the fog may not have lifted fully yet...

DiegoAndresCortes commented 1 year ago

I then suggest removing this https://github.com/SimpleMachines/SMF/blob/8a0b81895977fb29a2902aec39d555075764a05d/Sources/Subs-Editor.php#L1528-L1534

And using jquery.sceditor.theme.css (when it exists) in here, along with the things you are proposing? https://github.com/SimpleMachines/SMF/blob/8a0b81895977fb29a2902aec39d555075764a05d/Sources/Subs-Editor.php#L1912

In addition, should this 'style' setting be added after the hook then? Or is there any reason at all to edit this specific setting with a hook?

DiegoAndresCortes commented 1 year ago

I suppose there's also no reason to use a custom file at all and it would avoid breaking the source view for themes that used jquery.sceditor.theme.css The name of these files was indeed misleading I'd say as .default and .theme both do entirely different things 😕

sbulen commented 1 year ago

I would dynamically build both filenames. And update the docs on theme variants accordingly.

For the theme.css:

Similarly, for the default.css (wysiwyg):

The idea is to fully support sceditor within theme variants, with no code needed. And this way, it should be fully backwards compatible with existing themes as well.

Sesquipedalian commented 1 year ago

That's a well thought out plan, @sbulen. I like it. It should take care of the needs of theme authors very well.

There is one thing I would add to it for the sake of mod authors. Specifically, immediately after this line, I would like us to add the following:

if (typeof options.css === "string")
    instance.css(options.css);

This addition will enable mods to add CSS to the WYSIWYG mode of the editor using the integrate_sceditor_options hook. In their hooked function they'll just need to add their custom CSS to $sce_options['css'], and then those two new lines in jquery.sceditor.smf.js will take care of the rest.