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.

Sign up for GitHub

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

Jump to bottom

Make the getResizeObserver re-use a single observer instance for performance reasons #6145

Closed
oleq opened this issue Jan 27, 2020 · 2 comments · Fixed by ckeditor/ckeditor5-utils#320
Closed

Make the getResizeObserver re-use a single observer instance for performance reasons #6145

oleq opened this issue Jan 27, 2020 · 2 comments · Fixed by ckeditor/ckeditor5-utils#320
Assignees
@oleq
Labels
domain:ui/ux This issue reports a problem related to UI or UX. package:utils type:performance This issue reports a performance issue or a possible performance improvement.
Milestone
iteration 29 (v17...

Comments

@oleq
Copy link
Member

oleq commented Jan 27, 2020

Provide a description of the task

We used to have just one observer use–case for toolbar grouping. Now there's a new one for balloon toolbars and another one for the inline toolbar.

I figured this should be a performance issue, and it is. There should be a single instance used on a web page because otherwise the performance loss is huge.
@oleq oleq added package:utils domain:ui/ux This issue reports a problem related to UI or UX. type:performance This issue reports a performance issue or a possible performance improvement. labels Jan 27, 2020
oleq added a commit to ckeditor/ckeditor5-utils that referenced this issue Jan 28, 2020
@oleq
Other: Replaced the getResizeObserver() helper with a ResizeObserver …
f5e3384 
…class for performance reasons. Closes ckeditor/ckeditor5#6145.
This was referenced Jan 28, 2020
Merged
Merged
@oleq oleq added this to the iteration 29 milestone Jan 28, 2020
@Reinmar
Copy link
Member

Reinmar commented Jan 28, 2020

WUT, this is weird. How can we ensure that there's a single instance on the entire page? We don't control the whole page obviously. What if someone added more observers?

@oleq
Copy link
Member Author

oleq commented Jan 29, 2020

At least we can make sure there's only one observer added by CKEditor features and this is what my PR does.

@panr panr mentioned this issue Jan 29, 2020
Merged
panr added a commit to ckeditor/ckeditor5-utils that referenced this issue Feb 6, 2020
@panr
Merge pull request #320 from ckeditor/i/6145
05c97f8 
Other: Replaced the getResizeObserver() helper with the ResizeObserver class for performance reasons. See ckeditor/ckeditor5#6145.

MINOR BREAKING CHANGE: The getResizeObserver() helper is no longer available. We recommend using the ResizeObserver class instead.
@oleq oleq self-assigned this Feb 6, 2020
panr added a commit to ckeditor/ckeditor5-ui that referenced this issue Feb 6, 2020
@panr
Merge pull request #538 from ckeditor/i/6145
33a205d 
Internal: Aligned the ToolbarView class (and tests) to the ResizeObserver that replaced the getResizeObserver() helper (see ckeditor/ckeditor5#6145).
oleq added a commit to ckeditor/ckeditor5-utils that referenced this issue Feb 6, 2020
@oleq
Other: Replaced the getResizeObserver() helper with the `ResizeObse…
5ed4799 
…rver` class for performance reasons. Closes ckeditor/ckeditor5#6145.

MINOR BREAKING CHANGE: The `getResizeObserver()` helper is no longer available. We recommend using the [`ResizeObserver`](http://ckeditor.com/docs/ckeditor5/latest/api/module_utils_dom_resizeobserver-ResizeObserver.html) class instead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@oleq oleq

Labels
domain:ui/ux This issue reports a problem related to UI or UX. package:utils type:performance This issue reports a performance issue or a possible performance improvement.
Projects
None yet
Milestone
iteration 29 (v17.0.0)
2 participants
@Reinmar @oleq