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

Does maintaining a vanilla and editor Lark in parallel make any sense? #3395

Closed
oleq opened this issue Feb 9, 2018 · 6 comments · Fixed by ckeditor/ckeditor5-theme-lark#139
Assignees

Comments

@oleq
Copy link
Member

oleq commented Feb 9, 2018

Status of things

ATM we have the "vanilla" Lark (see theme MT):

image

and the "editor" Lark:

image

next to each other. The later is implemented by editorui.css.

Why having two sub–Larks is a bad idea?

Although in the past it made some sense because "generic" buttons (e.g. in the link balloon) had different styles than the toolbar

image

ATM there's hardly any difference

image

The problem is that maintaining both of them is a waste of time. I tried to fix https://github.com/ckeditor/ckeditor5-theme-lark/issues/134 ASAP but I soon found out that to make both "themes" fixed, it would take some serious refactoring in variables, etc.. The "editor" Lark overrides the "vanilla" one in many places and thus the whole thing becomes more and more complex as new components arrive.

If we assumed that Lark always looks the same (just like the editor toolbar), things would become more straightforward and we could save some (considerable) amount of time.

Why having two sub–Larks is a good idea?

Nothing comes to my mind.


cc @Reinmar @dkonopka @wwalc

@Reinmar
Copy link
Member

Reinmar commented Feb 12, 2018

TBH, I don't understand this ticket. Do you suggest that the vanilla theme makes no sense? Or that extending Lark is tough so it's hard to have two Lark-like styles at the same time?

@oleq
Copy link
Member Author

oleq commented Feb 12, 2018

Both, actually. For one thing, we don't use the vanilla theme. And the less we override in our default styles, the better. If the CSS we ship with the editor is already complex (overrides, etc.), developers will need to figure out the meaning of all the mess to put their overrides on top of it.

@Reinmar
Copy link
Member

Reinmar commented Feb 13, 2018

I don't know enough about these themes implementation to say something insightful here. But the idea of having a wireframe base theme + real themes on top of it is close to me. The question is how it worked in practice. Whether the issues that you mention are caused by the idea itself or flaws in the implementation.

@oleq
Copy link
Member Author

oleq commented Feb 13, 2018

It's not about the wireframe. We have 3 layers of the styling ATM:

  1. wireframes in packages (in the link, basic-styles, etc.),
  2. vanilla Lark in... Lark (first screenshot),
  3. editor UI styles in Lark (second screenshot).

I'd like to see the 2nd layer removed. It's never really used. E.g. maintaining a split dropdown so it looks OK

  • in the in the editor toolbar image
  • and in the vanilla theme image

costs us a lot, although we never use the later.

@Reinmar
Copy link
Member

Reinmar commented Feb 13, 2018

Ah, right. Now I understand it a bit more. So, I agree that maintaining a "working" components in the vanilla theme makes limited sense. Perhaps we need to define what each level should define and see how we can remove that one level.

@dkonopka
Copy link
Contributor

costs us a lot, although we never use the later.

...and it will cost more and more time if we will think about coming features like tables (panel with grid) and other more complex UI elements.

If we assumed that Lark always looks the same (just like the editor toolbar), things would become more straightforward and we could save some (considerable) amount of time.

☝️ agree!

@dkonopka dkonopka self-assigned this Feb 14, 2018
oleq referenced this issue in ckeditor/ckeditor5-theme-lark Feb 16, 2018
Other: Removed the "generic" layer of the theme to simplify it and improve the maintainability. Closes #135.

BREAKING CHANGE: The `.ck-editor-toolbar` CSS class has been removed.
BREAKING CHANGE: Various CSS variables (mostly colors) have been removed. Please make sure your code uses the latest theme API.
BREAKING CHANGE: From now on there's only one subset of the theme, aligned to the default look of CKEditor 5.
oleq referenced this issue in ckeditor/ckeditor5-image Feb 16, 2018
Other: Removed the `.ck-editor-toolbar` and `.ck-editor-toolbar-container` classes from the UI (see ckeditor/ckeditor5-theme-lark#135).
oleq referenced this issue in ckeditor/ckeditor5-editor-classic Feb 16, 2018
Other: Removed the `.ck-editor-toolbar` class from the toolbar (see ckeditor/ckeditor5-theme-lark#135).
oleq referenced this issue in ckeditor/ckeditor5-editor-inline Feb 16, 2018
Other: Removed the `.ck-editor-toolbar` class from the toolbar (see ckeditor/ckeditor5-theme-lark#135).
oleq referenced this issue in ckeditor/ckeditor5-ui Feb 16, 2018
Other: Removed the `.ck-editor-toolbar` and `.ck-editor-toolbar-container` classes (see ckeditor/ckeditor5-theme-lark#135).
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-theme-lark Oct 9, 2019
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 a pull request may close this issue.

3 participants