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

datepicker: memory leak #1852

Merged
merged 1 commit into from
Mar 23, 2020
Merged

datepicker: memory leak #1852

merged 1 commit into from
Mar 23, 2020

Conversation

jigar140291
Copy link
Contributor

@jigar140291 jigar140291 commented Jan 31, 2018 •
edited
Loading

Ticket: 15250

@jsf-clabot
Copy link

jsf-clabot commented Jan 31, 2018 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@fnagel fnagel self-assigned this Dec 10, 2019
@fnagel
Copy link
Member

fnagel commented Dec 10, 2019

Hey @jigar140291 thanks for your contribution and sorry for the delay.

Sounds reasonable to me and I'm able to debug this, but not within memory debugger functionality. Can you help me out here?

@arschmitz What do you think about this?

mgol
mgol reviewed Dec 30, 2019
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.

This makes sense to me but it'd perhaps be good to have a unit test verifying the field is cleared.

@fnagel
Copy link
Member

fnagel commented Jan 2, 2020

@jigar140291 Would you mind adding a test for this?

@jigar140291
Copy link
Contributor Author

@fnagel Sorry for the delay. I will look into the test cases.

@jigar140291
Copy link
Contributor Author

@fnagel I have added test cases. could you please review it.

@jigar140291
Copy link
Contributor Author

@mgol unit test case updated.

mgol
mgol previously approved these changes Jan 10, 2020
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.

Looks good to me! @fnagel @arschmitz what do you think?

@fnagel fnagel self-requested a review January 14, 2020 08:58
fnagel
fnagel requested changes Jan 14, 2020
View reviewed changes
Copy link
Member

@fnagel fnagel left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good to me! Only one thing left before merging:

  • Squash commit into one
  • Improve commit message (e.g. "Datepicker: Fix current instance memory leak and add tests")

I can do this for you too if needed.

jigar140291
jigar140291 commented Jan 14, 2020
View reviewed changes
Copy link
Contributor Author

@jigar140291 jigar140291 left a comment

Choose a reason for hiding this comment

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

Working on it. Thanks for the feedback.

@jigar140291
Copy link
Contributor Author

@fnagel it's done. could you please review and merge.

@fnagel
Copy link
Member

fnagel commented Jan 14, 2020

@jigar140291 Sorry but now there are even more commits. Wanna try again or should I do this for you?

@jigar140291
Copy link
Contributor Author

@fnagel seems like I squash it incorrectly. Could you do that for me. Thanks!!

@jigar140291
Copy link
Contributor Author

@fnagel I have squashed commits into one single commit. please check and merge.

@jigar140291
Copy link
Contributor Author

@arschmitz Could you please review and merge it.

@fnagel fnagel merged commit 817ce38 into jquery:master Mar 23, 2020
@mgol mgol added this to the 1.13 milestone Jun 10, 2020
@mgol mgol mentioned this pull request Aug 1, 2024
Merged
@mgol mgol mentioned this pull request Aug 14, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fnagel fnagel fnagel approved these changes

@arschmitz arschmitz arschmitz approved these changes

@mgol mgol mgol left review comments

Assignees

@fnagel fnagel

Labels
None yet
Milestone
1.13.0
Development

Successfully merging this pull request may close these issues.
5 participants
@jigar140291 @jsf-clabot @fnagel @arschmitz @mgol