Skip to content

[Dropzone] Remove event listeners on disconnect #1951

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

Merged
merged 1 commit into from
Jul 8, 2024
Merged

Conversation

aleho
Copy link
Contributor

@aleho aleho commented Jul 1, 2024

Q A
Bug fix? yes
New feature? no
License MIT

Events added in controller connect were not removed on disconnect. This would lead to events being triggered twice if a dropzone input is, e.g., moved in the DOM.

@carsonbot carsonbot added Bug Bug Fix Dropzone Status: Needs Review Needs to be reviewed labels Jul 1, 2024
@@ -3,10 +3,16 @@ import { Controller } from '@hotwired/stimulus';
class default_1 extends Controller {
connect() {
this.clear();
this.previewClearButtonTarget.addEventListener('click', () => this.clear());
this.inputTarget.addEventListener('change', (event) => this.onInputChange(event));
this.clear = this.clear.bind(this);
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it better to bind in initialize stage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, yes, didn't think about that!

I'm never binding in connect and always use properties.

class A {
    connect() {
        someElement.addEventListener('click', this._click);
    }

    _click = (event) => {
        
    };

@carsonbot carsonbot added Status: Reviewed Has been reviewed by a maintainer and removed Status: Needs Review Needs to be reviewed labels Jul 2, 2024
@kbond
Copy link
Member

kbond commented Jul 6, 2024

@aleho, is the failing translator test related to this change?

@aleho
Copy link
Contributor Author

aleho commented Jul 6, 2024

@kbond Afaik that's related to the bug introduced by a change to the string component (breaking various parts of Symfony and the UX demos too).

(#1950 and symfony/symfony#57612)

@kbond
Copy link
Member

kbond commented Jul 8, 2024

Good catch, thanks Alexander.

@kbond kbond merged commit d2347b7 into symfony:2.x Jul 8, 2024
34 of 35 checks passed
@aleho
Copy link
Contributor Author

aleho commented Jul 9, 2024

Thank you @kbond!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Bug Fix Dropzone Status: Reviewed Has been reviewed by a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants