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

Allow modifying toolbar's viewportTopOffset in runtime #9672

Closed
Reinmar opened this issue May 11, 2021 · 3 comments · Fixed by #10352
Closed

Allow modifying toolbar's viewportTopOffset in runtime #9672

Reinmar opened this issue May 11, 2021 · 3 comments · Fixed by #10352
Assignees
Labels
domain:dx This issue reports a developer experience problem or possible improvement. package:ui squad:core Issue to be handled by the Core team. type:improvement This issue reports a possible enhancement of an existing feature.

Comments

@Reinmar
Copy link
Member

Reinmar commented May 11, 2021

📝 Provide a description of the improvement

Currently, config.toolbar.viewportTopOffset can only be set when creating the editor.

The value then ends up in editor.ui.view.stickyPanel.viewportTopOffset that's readonly.

Could we make this property writable? Or, maybe even better, add a method/getter in the editor.ui.view or somewhere deeper? 

The problem I see is that the (in the runtime) viewportTopOffset is not a configuration of editor.ui.view.toolbar but editor.ui.view.stickyPanel which is rather poorly named (seems to have nothing to do with the toolbar). Even if we'll make it writable, discoverability will be relatively poor.

That might be an argument for adding a method to control this property somewhere up the chain (e.g. in editor.ui.view) but I dont' want to overcomplicate this so I have mixed feelings.

cc @oleq 


If you'd like to see this improvement implemented, add a 👍 reaction to this post.

@Reinmar Reinmar added the type:improvement This issue reports a possible enhancement of an existing feature. label May 11, 2021
@Reinmar Reinmar added this to the nice-to-have milestone May 11, 2021
@Reinmar Reinmar added squad:dx domain:dx This issue reports a developer experience problem or possible improvement. package:ui labels May 11, 2021
@oleq
Copy link
Member

oleq commented May 12, 2021

I'm OK with making editor.ui.view.stickyPanel.viewportTopOffset for ClassicEditor:

  • configurable first via config.toolbar.viewportTopOffset,
  • then writable in runtime.

but editor.ui.view.stickyPanel which is rather poorly named (seems to have nothing to do with the toolbar). Even if we'll make it writable, discoverability will be relatively poor.

I agree that the name is not perfect (tells you what but not why). It should be renamed to (sticky)toolbarContainer or something similar. We can keep  stickyPanel for backward compatibility (and warn about deprecation).


Still, keep in mind that InlineEditor also uses config.toolbar.viewportTopOffset but does not have the sticky panel instance. So editor.ui.view.toolbarContainer.viewportTopOffset will not work as InlineEditorUIView uses BalloonPanelView available under editor.ui.view.panel. In this case, I'd also rename it to toolbarContainer to keep things similar across editor types and, similarly, have editor.ui.view.toolbarContainer.viewportTopOffset writable.

Proposal

  • editor.ui.view.stickyPanel -> editor.ui.view.toolbarContainer (Classic)
    • + warn when using deprecated property
  • editor.ui.view.panel -> editor.ui.view.toolbarContainer (Inline)
    • + warn when using deprecated property
  • configure editor.ui.view.toolbarContainer.viewportTopOffset using config.toolbar.viewportTopOffset 
  • make editor.ui.view.toolbarContainer.viewportTopOffset writable (and probably observable for the positioning logic to react to the change immediately)

@Reinmar
Copy link
Member Author

Reinmar commented May 14, 2021

@mlewand @mateuszzagorski There are some tips regarding the direction from Olek so you can consider giving it a try :)

@Reinmar Reinmar modified the milestones: nice-to-have, iteration 45 Jul 5, 2021
@Reinmar Reinmar added the squad:core Issue to be handled by the Core team. label Jul 19, 2021
@marcelvarela
Copy link

I would also like to see this implemented. I have an app with a different header size on mobile and desktop but this is a problem because ckeditor does not allow changing viewportTopOffset at runtime.

@AnnaTomanek AnnaTomanek modified the milestones: iteration 46, iteration 47 Sep 6, 2021
Reinmar added a commit that referenced this issue Sep 6, 2021
…et-in-runtime

Feature: Introduced `editor.ui.viewportOffset` that allows modifying the viewport's offset in runtime. This value is used by the editor to e.g. position its sticky toolbar and contextual balloons. Additionally, the `config.toolbar.viewportTopOffset` property was moved to `config.ui.viewportOffset` and it now accepts an object. Closes #9672.

BREAKING CHANGE: The `config.toolbar.viewportTopOffset` property was moved to `config.ui.viewportOffset` and it now accepts an object.
jackylauu pushed a commit to jackylauu/ckeditor5 that referenced this issue Nov 2, 2021
…rTopOffset-in-runtime

Feature: Introduced `editor.ui.viewportOffset` that allows modifying the viewport's offset in runtime. This value is used by the editor to e.g. position its sticky toolbar and contextual balloons. Additionally, the `config.toolbar.viewportTopOffset` property was moved to `config.ui.viewportOffset` and it now accepts an object. Closes ckeditor#9672.

BREAKING CHANGE: The `config.toolbar.viewportTopOffset` property was moved to `config.ui.viewportOffset` and it now accepts an object.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:dx This issue reports a developer experience problem or possible improvement. package:ui squad:core Issue to be handled by the Core team. type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants