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

Multiline toolbar (wrapping toolbar) #6146

Closed
thorleifjacobsen opened this issue Jan 27, 2020 · 19 comments · Fixed by #8507
Closed

Multiline toolbar (wrapping toolbar) #6146

thorleifjacobsen opened this issue Jan 27, 2020 · 19 comments · Fixed by #8507
Assignees
Labels
domain:ui/ux This issue reports a problem related to UI or UX. package:ui squad:core Issue to be handled by the Core team. support:2 An issue reported by a commercially licensed client. type:feature This issue reports a feature request (an idea for a new functionality or a missing option).

Comments

@thorleifjacobsen
Copy link

thorleifjacobsen commented Jan 27, 2020

Would love to see a possibility as CKEditor4 to have "/" as a toolbar item which breaks it to a new line when using the option "shouldNotGroupWhenFull"


If you'd like to see this feature implemented add 👍 reaction to this post.

@thorleifjacobsen thorleifjacobsen added the type:feature This issue reports a feature request (an idea for a new functionality or a missing option). label Jan 27, 2020
@Reinmar Reinmar added status:confirmed domain:ui/ux This issue reports a problem related to UI or UX. package:ui labels Jan 30, 2020
@Reinmar
Copy link
Member

Reinmar commented Jan 30, 2020

Thanks for the feature request. I guess we're doing everything to avoid having multiline toolbars (e.g. by introducing toolbar grouping), but let's see how many people would still prefer this way of handling big number of buttons.

@Reinmar Reinmar added this to the backlog milestone Jan 30, 2020
@Reinmar
Copy link
Member

Reinmar commented Feb 21, 2020

It's possible to create line breaks by:

  1. Registering a component named e.g. "/" which will render something like a <span class="break-line">.
  2. Styling it to something like flex-basis:100%; height:0; margin:0; padding:0.
  3. Styling the entire .ck-toolbar__items with flex-wrap: wrap.
  4. Once this is done, you can use a toolbar item called "/" to create line breaks.

@Reinmar
Copy link
Member

Reinmar commented Aug 18, 2020

I've been looking at demos of "document presets" such as https://ckeditor.com/docs/ckeditor5/latest/features/export-pdf.html and the buttons don't fit there even despite the wide column.

I think we need to prioritize wrapping the toolbar finally.

Also, we changed the styling of the vertical separator which makes that simpler now.

The tricky bit here is how to implement the separator as the child of the toolbar collection in a way that the focus cycler will ignore it.

We also need to disable toolbar wrapping for such toolbars. It does not make sense to try to make both features compatible with each other.

@Reinmar Reinmar changed the title CKEditor Toolbar - Line break Multiline toolbar (wrapping toolbar) Sep 21, 2020
@Reinmar Reinmar added the support:2 An issue reported by a commercially licensed client. label Sep 29, 2020
@Reinmar
Copy link
Member

Reinmar commented Sep 29, 2020

  • Automatic vs manual (declarative) toolbar wrapping.
    • Declarative is more predictable.
    • Automatic may end up with 18 items in the first line and 2 in the second. Would anyone be happy with that?
  • We need to document that this does not work with "group when full" so requires using "shouldNotGroupWhenFull".
    • Perhaps we can even automate this so when there's / in the toolbar, we disable grouping automatically.
  • We should document that this is not optimal when considering small mobile screens.
  • Future: automatically switch grouping on when hitting the "small screen starts wrapping 2 lines into 4 🤦 case".
  • Future: each line grouped separately 🔥. Report a followup.
    • Worth researching how similar editors (with similar toolbars) behave

@Reinmar Reinmar modified the milestones: backlog, iteration 37 Sep 29, 2020
@AnnaTomanek AnnaTomanek modified the milestones: iteration 37, nice-to-have Oct 19, 2020
@Reinmar Reinmar modified the milestones: nice-to-have, iteration 38 Oct 26, 2020
@oleq oleq self-assigned this Nov 17, 2020
@oleq
Copy link
Member

oleq commented Nov 17, 2020

  • PoC in i/6146-multiline-toolbar
  • I decided to leave the toolbar auto-grouping enabled for all editors by default for backward compatibility.
    • In other words, for the toolbar line break to work shouldNotGroupWhenFull: true must be specified in the toolbar configuration.
    • See (Problems) below to learn more.
  • I went with '/' for toolbar break in the toolbar configuration.
toolbar: {
    items: {
        'bold',
        'italic',
        'underline',
        'strikethrough',
        '/',
        'alignment',
        '|',
        'numberedList',
        'bulletedList',
    }
}
  • (Feedback needed) Let's make sure '/' is the right direction (future-proof). For instance, we're going to implement proper button groups sooner or later. Would the following make sense?
toolbar: {
    items: {
        { name: 'basic styles', items: [ 'bold', 'italic', 'underline', 'strikethrough' ] },
        '/',
        { name: 'other group', items: [ 'alignment', '|', 'numberedList', 'bulletedList' ] }
    }
}
  • (Task) Let's figure out the default toolbar layout for document build. I went with something like this and let's say I'm not thrilled about this

Even if we went back to the full-height separators and added some border to the line separator, this does not look great

  • I tried adding more features to the toolbar and IMO it looks better (for instance, individual alignment buttons). I also slightly re-arranged the buttons for better semantics.
    • Unfortunately, this will increase the side of the build.
    • OTOH this kind of build should offer all features a traditional native word processor would offer so this course of action makes sense to me.
    • This layout looks good enough to me.

  • (Problem) I wanted  ToolbarView to be smarter and disable automatic item grouping whenever the line break appears in the configuration.
    • It turned out not so simple because the behavior of the toolbar (static vs. auto-grouping) is applied in the constructor when the items of the toolbar are unknown.
    • ToolbarView#fillFromConfig() can be called at any time (but after constructor() and render()) and only then the toolbar would know if it can enable auto-grouping or not depending on the presence of line breaks in the config.
    • Re-writing the logic so the toolbar can adapt to the config when it's already rendered is a major refactoring of the entire component. We didn't see that coming when we first implemented auto-grouping.
    • TBH for this all to make sense, the toolbar would need an ability to switch between auto-grouping and static (break-compatible) layout on every change of it's #items collection. Developers can use ToolbarView#fillFromConfig() but they may also fill the toolbar with items manually by adding to the #items collection. It could be that the third button added to the toolbar is a line break and the auto-grouping feature must be from now on disabled.
      • I estimate this is at least a couple of MD of work and I'd rather see this as a follow-up.
  • (Problem) I tried multi-line toolbar in InlineEditor (but the same applies to the ballon editor etc.) and it turns out all floating toolbars use the .ck-toolbar_floating class that disables flex-wrap. Without flex-wrap multi-line toolbars won't break at a specific point.
    • Disabling flex-wrap is necessary for auto-grouping (enabled by default) to work, though. So here we've got a problem.
    • I thought this would be as easy as getting rid of the .ck-toolbar_floating class whenever a line break appears in ToolbarView#items but the class is added externally by editor UI implementations (like InlineEditorUIView) via extendTemplate() and ToolbarView does not know about it.
    • The only solution I see now is passing a property like isFloating to the ToolbarView constructor and allowing the toolbar to decide whether the CSS class is added
      • .ck-toolbar_floating would be set only when constructorOptions.shouldGroupWhenFull === true and constructorOptions.isFloating === true

@Reinmar
Copy link
Member

Reinmar commented Nov 17, 2020

I went with '/' for toolbar break in the toolbar configuration.

That's what CKE4 uses, so it may be a simpler upgrade path.

  • (Idea) We could go with '-' instead because it is clearly horizontal.

Makes sense too.

  • (Feedback needed) Let's make sure '/' is the right direction (future-proof). For instance, we're going to implement proper button groups sooner or later. Would the following make sense?

It worked for CKE4 (that had groups too), so it seems fine.

  • I tried adding more features to the toolbar and IMO it looks better

Yeah, I think I agree that there needs to be a balance between a too empty toolbar and too wide. In the end, this will be the integrator's choice anyway as they will choose the included features, but we should have some reasonable defaults.

I don't have problems with extending the document build. The demo that we have on https://ckeditor.com/ckeditor-5/demo/#document has more features anyway. I'd run this by @wwalc as he'll have the most input.

(Problem) I wanted  ToolbarView to be smarter and disable automatic item grouping whenever the line break appears in the configuration.

What if fillFromConfig() checked the current state of the toolbar once it is about to add the '-' item and log a warning? That's not going to catch all situations and it will not resolve them automatically, but will give the developer a cue in the most probable scenario.

  • The only solution I see now is passing a property like isFloating to the ToolbarView constructor and allowing the toolbar to decide whether the CSS class is added

Sounds reasonable.

@oleq
Copy link
Member

oleq commented Nov 17, 2020

TODOs

@Reinmar
Copy link
Member

Reinmar commented Nov 17, 2020

Pinged @wwalc for feedback. Let's gather it before we'll move forward to avoid surprises.

@oleq
Copy link
Member

oleq commented Nov 17, 2020

Food for thought: here's the document editor build in action on a 1280x800 laptop screen 

  • with the declarative multi-line toolbar 
  • and toolbar items flowing to the next line

@Reinmar
Copy link
Member

Reinmar commented Nov 17, 2020

Another TODO: we need to support line breaks in https://ckeditor.com/ckeditor-5/online-builder/. It's not a blocker for the release, but should be resolved in a near future.

@wwalc
Copy link
Member

wwalc commented Nov 17, 2020

  • Let's go with declarative multi-line toolbar for the document editor build. I will recheck what features could land there taking this occasion that we will have more space there.
  • The last design looks the best IMHO (the one without line separating toolbar lines).
  • Our online builder will have to support multiline toolbar - there may a simple way to implement support - e.g. just render 3-4 pseudo-toolbars instead of one and include them in the configuration if at least one button is added in a toolbar (well, a little bit troublesome may be rendering the initial state of the toolbar in the online builder UI if one enabled all plugins). Such a pseudo toolbar could be grayed out in the UI until at least one button is placed in it. Anyway, just a quick idea, there are probably many ways how we can quickly add it.
  • As for / vs - I'd roll a dice, because / will be less confusing for users upgrading from CKEditor 4, while - seems to be more intuitive.
  • While we agree that there are many problems still left to be resolved (mobiles etc.), we can just ignore them for now. E.g. I imagine that on systems where the document editor is used, a minimum length of the document (and the space for the editor toolbar) will be enforced, so a proper toolbar configuration that takes into consideration the minimal space for the toolbar will prevent users from seeing an editor with malformed toolbar. I'd just start gathering ideas on what direction we could take in the future with the toolbar further, but keep it in the backlog due to other priorities.

@Reinmar
Copy link
Member

Reinmar commented Nov 17, 2020

BTW, @oleq what about #5586? Can we add it to the TODOs?

@pkwasnik
Copy link
Contributor

In paste from word/google and export to pdf we have only one/two extra buttons, so the multiline toolbar will look not so good. I propose just to remove those buttons to make it fit. I could also go the other way and add more buttons, but I am not sure if we have to show multiline in those demos. One line always looks better IMO.
obraz.png
VS
obraz.png
OR
obraz.png

About the separator: both are good, but my vote is slightly towards -. / looks too similar to |. When creating the config it is easier to see the difference when there is -.

@Reinmar
Copy link
Member

Reinmar commented Nov 23, 2020

TODO:

  • Document the line separator in the migration guide.
  • Link to the new toolbar guide from the configuration guide.
  • Move all works on the documentation to a separate ticket.

@pkwasnik
Copy link
Contributor

pkwasnik commented Nov 23, 2020

Follow up about "Toolbar configuration" guide: #8513

EDIT: I've closed follow up, as I've included docs in this branch.

@oleq
Copy link
Member

oleq commented Nov 26, 2020

Can we address #6146 (comment)? This is the main reason why we're working on the multi-line toolbar. 

Personally, I'm for a multiple-line declarative toolbar with as many features as possible to mimic the document build. 

cc @Reinmar

@vishwakarmanikhil
Copy link

when has this issue gone be solved? waiting for the next update of ckEditor5.

@AnnaTomanek
Copy link
Contributor

Follow-up for the docs: #8493

@oleq oleq closed this as completed in #8507 Dec 1, 2020
oleq added a commit that referenced this issue Dec 1, 2020
Feature (ui, theme-lark): Implemented a toolbar configuration that allows rendering toolbar items in multiple rows. Closes #6146.

Feature (build-decoupled-document): Added new features to the build configuration (horizontal line, page break, remove formatting, and special characters) (see #6146).

Internal (editor-inline): Aligned the editor view to the latest API of `ToolbarView` (see #6146).

Docs (core): Explained how to configure the editor toolbar to make it break in multiple lines (see #6146).
@pkwasnik
Copy link
Contributor

pkwasnik commented Dec 2, 2020

Follow-up for supporting line breaks in online builder: #8567

Follow-up for updating the document editor demo: cksource/bigbang-ckeditor.com#2217

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:ui/ux This issue reports a problem related to UI or UX. package:ui squad:core Issue to be handled by the Core team. support:2 An issue reported by a commercially licensed client. type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants