Opened 10 years ago
Closed 10 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 10 years ago by
Status: | new → confirmed |
---|
comment:2 Changed 10 years ago by
Owner: | set to Artur Delura |
---|---|
Status: | confirmed → assigned |
comment:3 Changed 10 years ago by
comment:4 Changed 10 years ago by
Status: | assigned → review |
---|
Changes in branch:t/13147. I added one container which stick together all floating elements.
comment:5 Changed 10 years ago by
Status: | review → review_failed |
---|
- Use element.getName(), not element.$.tagName.toLowerCase(). Plus - is it necessary at all?
- There are unsused width and height variables inside the check() method.
- There's no blank line between last two methods.
- 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 10 years ago by
Status: | review_failed → assigned |
---|
comment:7 Changed 10 years ago by
According to point 4 - yes, there is an easier way to do this. Changes in branch:t/13147.
comment:8 Changed 10 years ago by
Status: | assigned → review |
---|
comment:9 Changed 10 years ago by
Milestone: | CKEditor 4.5.0 Beta → CKEditor 4.5.0 |
---|
comment:10 Changed 10 years ago by
Component: | General → Toolbar Configurator |
---|
comment:11 Changed 10 years ago by
Resolution: | → fixed |
---|---|
Status: | review → closed |
Great job. Fixed on major with git:363bdfe.
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.