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

Resize observer #5095

Merged
merged 1 commit into from
Mar 22, 2023
Merged

Resize observer #5095

merged 1 commit into from
Mar 22, 2023

Conversation

nightwing
Copy link
Member

users too often forget to call editor.resize after changing the size of the editor.
Since ResizeObserver is now supported on all browsers we can check for size changes ourselves.
I have added an option in case someone needs more fine grained control over resize during the animations.

includes #5094 to use the dialog in kitchen-sink added by it.

@codecov
Copy link

codecov bot commented Mar 19, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.02 🎉

Comparison is base (a2e89b9) 86.60% compared to head (51d5e4d) 86.62%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5095      +/-   ##
==========================================
+ Coverage   86.60%   86.62%   +0.02%     
==========================================
  Files         555      555              
  Lines       42931    42996      +65     
  Branches     6697     6709      +12     
==========================================
+ Hits        37181    37247      +66     
+ Misses       5750     5749       -1     
Flag Coverage Δ
unittests 86.62% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/ace.js 100.00% <ø> (ø)
src/editor.js 82.93% <ø> (ø)
src/ace_test.js 99.24% <100.00%> (+0.37%) ⬆️
src/virtual_renderer.js 80.81% <100.00%> (+0.56%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@InspiredGuy InspiredGuy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. you could probably improve test coverage a bit, since there are come uncovered lines, but overall looks good.

@@ -66,9 +66,7 @@ exports.edit = function(el, options) {
onResize: editor.resize.bind(editor, null)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we remove onResize from env here? I see that it's used in doc site https://github.com/ajaxorg/ace/blob/master/doc/site/js/main.js#L158
can we rely on new functionality, introduced in this PR, in that place as well, instead of manually calling onResize?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, we can remove it from main.js, but i am not sure about env.onResize, because on one hand it is a breaking change, but on other hand it is obscure api.

@nightwing nightwing force-pushed the resize-observer branch 4 times, most recently from 9416319 to c083800 Compare March 22, 2023 22:04
@nightwing nightwing merged commit 9f86bb0 into master Mar 22, 2023
@nightwing nightwing deleted the resize-observer branch March 22, 2023 22:19
@nightwing
Copy link
Member Author

Got the coverage to 100%

@shepmaster
Copy link

Thanks for this ❤️ ! This will allow me to remove some ugly code from my code base that existed solely to call resize at the appropriate time.

shepmaster added a commit to rust-lang/rust-playground that referenced this pull request Jul 11, 2023
[Ace added a `ResizeObserver`][pr-5095] to automatically perform
resizes as needed, so we can remove our junk — hooray!

[pr-5095]: ajaxorg/ace#5095
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants