Opened 3 years ago

Closed 3 years ago

#13147 closed New Feature (fixed)

[Toolbar configurator] Add buttons to the sticky toolbar

Reported by: Piotrek Koszuliński Owned by: Artur Delura
Priority: Normal Milestone: CKEditor 4.5.0
Component: Toolbar Configurator Version: 4.5.0 Beta
Keywords: Cc:

Description

In #13135 we made the toolbar sticky, now it's time to add buttons to it.

My advice would be to make only one element with fixed position and appending the elements that should be fixed to it. The reason is that the div with toolbar may change height dynamically so setting top position of buttons would be hard. It'll be better when they'll be in the same container.

Change History (11)

comment:1 Changed 3 years ago by Piotrek Koszuliński

Status: newconfirmed

comment:2 Changed 3 years ago by Artur Delura

Owner: set to Artur Delura
Status: confirmedassigned

comment:3 Changed 3 years ago by Artur Delura

Wes should add configurator headers as well. I don't think we should keep all elements in one container. If we move elements to a different container they might loss some styles (parent child selectors). Another thing is if previous element which is pinned decreased it's height then next element might become unpinned so we have to execute some code to check it. The other way around is previous element height increase it's height then other element might becomes pinned.

So we have to know when some element changes it's dimenstion and execute proper method to check and update everything.

comment:4 Changed 3 years ago by Artur Delura

Status: assignedreview

Changes in branch:t/13147. I added one container which stick together all floating elements.

comment:5 Changed 3 years ago by Piotrek Koszuliński

Status: reviewreview_failed
  1. Use element.getName(), not element.$.tagName.toLowerCase(). Plus - is it necessary at all?
  2. There are unsused width and height variables inside the check() method.
  3. There's no blank line between last two methods.
  4. What does getCurrentTopOffset() do? It doesn't look right at all... Iterating over the entire array except the last element if the current element is fixed? Summing up only the heights of the elements, without margins? It's very weird and should be commented to pass review. But is it needed at all? Isn't it enough to check the height of the sticky container and check whether element's (or its placeholder) top position isn't lower than that height?

comment:6 Changed 3 years ago by Artur Delura

Status: review_failedassigned

comment:7 Changed 3 years ago by Artur Delura

According to point 4 - yes, there is an easier way to do this. Changes in branch:t/13147.

comment:8 Changed 3 years ago by Artur Delura

Status: assignedreview

comment:9 Changed 3 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.5.0 BetaCKEditor 4.5.0

comment:10 Changed 3 years ago by Wiktor Walc

Component: GeneralToolbar Configurator

comment:11 Changed 3 years ago by Piotrek Koszuliński

Resolution: fixed
Status: reviewclosed

Great job. Fixed on major with git:363bdfe.

Note: See TracTickets for help on using tickets.
© 2003 – 2017 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy