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

Resizable: Fix resizing of elems with box-sizing: border-box #2012

Merged
merged 9 commits into from
Oct 10, 2022
Merged

Resizable: Fix resizing of elems with box-sizing: border-box #2012

merged 9 commits into from
Oct 10, 2022

Conversation

mcanepa
Copy link
Contributor

@mcanepa mcanepa commented Nov 9, 2021

fixes #1979
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 9, 2021 •
edited
Loading

CLA Signed

The committers are authorized under a signed CLA.
@mgol
Copy link
Member

mgol commented Dec 2, 2021

We had broken CI for a while. Please rebase to the latest main so that GitHub Actions run on the PR.

We'll also need unit tests for the change and I'll need some time to understand what's exactly going on here; although, if you could add comments on GitHub explaining the code changes, that'd speed things up.
@mgol mgol closed this Jan 1, 2022
@mgol mgol reopened this Jan 1, 2022
@mgol
Copy link
Member

mgol commented Jan 5, 2022

If you're interested in finishing this, there are some existing autoResize tests in tests/unit/resizable/options.js, you'd need to add a new test there.
@mcanepa
Copy link
Contributor Author

mcanepa commented Jan 5, 2022

I'll try my best...

This will be my first encounter with a unit test so any help from anyone it would be very much appreciated
@mgol
Copy link
Member

mgol commented Jan 5, 2022

It's best to look at existing tests and do something similar. Of course, feel free to ask questions.
@mcanepa
Copy link
Contributor Author

mcanepa commented Jan 30, 2022

@mgol just added a test. I hope it's ok!
@mgol mgol removed the Needs info label Feb 8, 2022
@mcanepa
Copy link
Contributor Author

mcanepa commented Jul 30, 2022

hello @mgol @fnagel

Any chance for this issue to be reviewed?
mgol
mgol requested changes Oct 5, 2022
View reviewed changes
Copy link
Member

@mgol mgol left a comment

Choose a reason for hiding this comment

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

See my comments. Please also merge the latest main to your branch. When that gets addressed, I'll approve the PR.

@fnagel do you have any other concerns or are you also OK with landing this after my remarks are addressed?
ui/widgets/resizable.js Outdated Show resolved Hide resolved
ui/widgets/resizable.js Outdated Show resolved Hide resolved
mcanepa and others added 3 commits October 5, 2022 09:47
@mcanepa
Merge branch 'jquery:main' into main
e5281af
@mcanepa @mgol
Update ui/widgets/resizable.js
9019f29 
Co-authored-by: Michał Gołębiowski-Owczarek <m.goleb@gmail.com>
@mcanepa @mgol
Update ui/widgets/resizable.js
76e0476 
Co-authored-by: Michał Gołębiowski-Owczarek <m.goleb@gmail.com>
mgol
mgol approved these changes Oct 5, 2022
View reviewed changes
Copy link
Member

@mgol mgol left a comment

Choose a reason for hiding this comment

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

Thank you! Looks good to me now. I'll still wait a bit for @fnagel's review before merging.
@mgol mgol requested a review from fnagel October 5, 2022 14:02
@mgol mgol removed the Needs tests label Oct 5, 2022
fnagel
fnagel approved these changes Oct 10, 2022
View reviewed changes
Copy link
Member

@fnagel fnagel left a comment

Choose a reason for hiding this comment

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

+1 by reading
@mgol mgol added this to the 1.13.3 milestone Oct 10, 2022
@mgol mgol changed the title Resizable: Fix issue with border-box Oct 10, 2022
@mgol mgol merged commit 62f2ccc into jquery:main Oct 10, 2022
@mgol
Copy link
Member

mgol commented Oct 10, 2022

Landed, thanks!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Behavior shared with 1.12 Bug Comp: Dialog
3 participants
@mcanepa @mgol @fnagel