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

Feature split: which part of a feature should import content style CSS file? #974

Closed
jodator opened this issue Apr 18, 2018 · 9 comments
Closed
Labels
resolution:expired This issue was closed due to lack of feedback. status:discussion type:question This issue asks a question (how to...).

Comments

@jodator
Copy link
Contributor

jodator commented Apr 18, 2018

So we did a feature split (#488) some time ago which created FeatureEditing and FeatureUI parts for some general Feature.

The thing that bothers me is whether content styles (to be precise: styles for markup in the editor not the UI elements) should be imported by FeatureUI or by EditingUI.
The examples are:

  • FontSize
  • FontFamily
  • Highlight
  • probably Table (styling a widget)
  • Blockquote (?)
  • ... other?
    If you import for instance HighlightEditing only and you'd like to create own UI the content styles defined by configuration (colors of highlighters) will not be created as HighlightUI imports css file?

As such I'm leaning towards that we should move those content styles imports to editing part of a feature as those are tied to editing part of a feature.

Side note: I have some problems with defining whether Widget is UI part or Editing part of the editor. The Widget plugin itself has no Editing nor UI parts extracted. But as long it operates on editable content I think that it is an Editing stuff. So if some feature modifies styles for widget (Table) those styles should be imported by it Editing part.

@jodator jodator added type:question This issue asks a question (how to...). status:discussion labels Apr 18, 2018
@oleq
Copy link
Member

oleq commented Apr 20, 2018

I think that CSS is related to the editing more than to the UI. One could create an editor with their custom UI and use our default content styles (like the font or highlight styles). That makes sense.

OTOH, that kind of CSS is irrelevant when they create a headless editor. E.g. on the server-side. They could probably filter all the CSS out at the Webpack stage but still the CSS imports in *Editing could be perplexing.

So it gives us 2 options:

  1. Put them in the feature.
  2. Put them in the editing and expect server–side integrations will filter them out.

What are the drawbacks of 1.?

cc @Reinmar

@jodator
Copy link
Contributor Author

jodator commented Apr 20, 2018

@oleq I'd rather see them in Editing and be filtered out by webpack for headless. What if someone use Editing + UI without a main feature? Or use different UI (like heading buttons) with existing editing?

@scofalik
Copy link
Contributor

I wouldn't be mad if those styles would in in Editing.

@oleq
Copy link
Member

oleq commented Apr 20, 2018

What if someone use Editing + UI without a main feature?

Hm. A good point. OTOH, why not use our glue feature class in this case?

Or use different UI (like heading buttons) with existing editing?

They could use the same import statement for the CSS that we have ATM. It would point at the same content CSS file.

@Reinmar
Copy link
Member

Reinmar commented Jun 20, 2018

If the editing part of the feature would import these CSS files, then we would lose Node.js compatibility. We tested some time ago that you can run CKEditor 5's source in Node.js without building it. We didn't run the whole editor, but I expect that unless you import any SVG or CSS files, that should be possible.

However, it's only possible as long as we keep the strict UI vs Editing separation. I checked now that SVG and CSS files are mainly imported by UI modules. There are two exceptions:

  • imagestyle/utils.js – easy to fix, just move the icons elsewhere,
  • widget.js – tricky, because styles are related to the editing area; just like content styles, but harder to separate.

So, theoretically, if not for cases like widgets (where we have a UI inlined in the editing area), it would be easy. The only thing which would bother me is that the UI modules deal with UI of the features. While content styles are not part of that. So I could agree to import them in the Feature part of the FeartureUI, FeatureEditing, Feature trio. Perhaps it would be then more evident what you need to import (editing+CSS) when you replace our UI modules.

However, styles and SVGs used directly in the editing area are more tricky. I'm quite sure that we could come up with a way to split the code of e.g. the Widget plugin to clean this up (e.g. having WidgetEditing and WidgetUIImageEditing would import the former and ImageUI the latter) but I think it's too early to work on this. Let's wait until we really need to run the code in Node.js.


To sum up:

  • I'm strongly against merging CSS into the FeatureEditing part.
  • I'm ok with moving CSS to the Feature part.
  • But that still doesn't solve the problem of SVGs and CSS used inline in the editing area.

@Reinmar Reinmar added this to the unknown milestone Jun 20, 2018
@jodator
Copy link
Contributor Author

jodator commented Jun 21, 2018

If the editing part of the feature would import these CSS files, then we would lose Node.js compatibility. We tested some time ago that you can run CKEditor 5's source in Node.js without building it. We didn't run the whole editor, but I expect that unless you import any SVG or CSS files, that should be possible.

Ah yes - it's a blocker for importing CSS/SVG's there. Right now I can't think about anything else then extracting some sort of FeatureEditingUI part and importing it in a Feature along with FeatureEditing.

Also I agree that Node.js compatibility and/or headless editor is more important then how to organize the code.

@CKEditorBot
Copy link
Collaborator

There has been no activity on this issue for the past year. We've marked it as stale and will close it in 30 days. We understand it may be relevant, so if you're interested in the solution, leave a comment or reaction under this issue.

@CKEditorBot
Copy link
Collaborator

There has been no activity on this issue for the past year. We've marked it as stale and will close it in 30 days. We understand it may still be relevant, so if you're interested in the solution, leave a comment or reaction under this issue.

@CKEditorBot
Copy link
Collaborator

We've closed your issue due to inactivity over the last year. We understand that the issue may still be relevant. If so, feel free to open a new one (and link this issue to it).

@CKEditorBot CKEditorBot added resolution:expired This issue was closed due to lack of feedback. and removed status:stale labels Jan 15, 2024
@CKEditorBot CKEditorBot closed this as not planned Won't fix, can't repro, duplicate, stale Jan 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
resolution:expired This issue was closed due to lack of feedback. status:discussion type:question This issue asks a question (how to...).
Projects
None yet
Development

No branches or pull requests

6 participants