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

Filterable backcompat: Trigger listviewbeforefilter #6823

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@gabrielschulhof
Contributor

gabrielschulhof commented Dec 20, 2013

Fixes #6679

@arschmitz

This comment has been minimized.

Show comment
Hide comment
@arschmitz

arschmitz Dec 23, 2013

Member

several questions here.

  • Why are we listening for an even emitted by the same widget?
  • Why arnt you using this._on doing it with .on makes it so you are delegating this to the document and ._on avoids the need for the proxy
  • Why are we checking event.target this this an attempt to support nested filterable? if so do have tests to show this works at all or have we ever talked about if this is supported? also not listing for the event and calling directly would avoid the need for this
Member

arschmitz commented on js/widgets/filterable.backcompat.js in e110656 Dec 23, 2013

several questions here.

  • Why are we listening for an even emitted by the same widget?
  • Why arnt you using this._on doing it with .on makes it so you are delegating this to the document and ._on avoids the need for the proxy
  • Why are we checking event.target this this an attempt to support nested filterable? if so do have tests to show this works at all or have we ever talked about if this is supported? also not listing for the event and calling directly would avoid the need for this
@arschmitz

This comment has been minimized.

Show comment
Hide comment
@arschmitz

arschmitz Dec 23, 2013

Member

triggering an event like this in another widgets name seems bad.

Member

arschmitz commented on js/widgets/filterable.backcompat.js in e110656 Dec 23, 2013

triggering an event like this in another widgets name seems bad.

@arschmitz

This comment has been minimized.

Show comment
Hide comment
@arschmitz

arschmitz Dec 23, 2013

Member

This is missing tests to prevent regression

Member

arschmitz commented Dec 23, 2013

This is missing tests to prevent regression

@gabrielschulhof

This comment has been minimized.

Show comment
Hide comment
@gabrielschulhof

gabrielschulhof Jan 2, 2014

Contributor

To answer your questions:

  1. "Why are we listening for an even emitted by the same widget?"

    The filterable widget does not provide a extension point where it emits this event, nor should it, because this is for backcompat only, so we shouldn't be modifying the non-deprecated code to accommodate the deprecated code.

  2. "Why arnt you using this._on doing it with .on makes it so you are delegating this to the document and ._on avoids the need for the proxy"

    Fixed that.

  3. "Why are we checking event.target this this an attempt to support nested filterable? if so do have tests to show this works at all or have we ever talked about if this is supported? also not listing for the event and calling directly would avoid the need for this"

    I'm not attempting to support it, but I have realized that people might nest these, and the one scenario where a bubbling filterablebeforefilter causes some parent listview that is also filterable to erroneously trigger this event is to be avoided. I guess, unless we explicitly say we don't support it, people will do whatever they want, as long as the markup is valid - and sometimes even when it isn't :/

    I don't quite understand "also not listing for the event and calling directly would avoid the need for this"

Contributor

gabrielschulhof commented Jan 2, 2014

To answer your questions:

  1. "Why are we listening for an even emitted by the same widget?"

    The filterable widget does not provide a extension point where it emits this event, nor should it, because this is for backcompat only, so we shouldn't be modifying the non-deprecated code to accommodate the deprecated code.

  2. "Why arnt you using this._on doing it with .on makes it so you are delegating this to the document and ._on avoids the need for the proxy"

    Fixed that.

  3. "Why are we checking event.target this this an attempt to support nested filterable? if so do have tests to show this works at all or have we ever talked about if this is supported? also not listing for the event and calling directly would avoid the need for this"

    I'm not attempting to support it, but I have realized that people might nest these, and the one scenario where a bubbling filterablebeforefilter causes some parent listview that is also filterable to erroneously trigger this event is to be avoided. I guess, unless we explicitly say we don't support it, people will do whatever they want, as long as the markup is valid - and sometimes even when it isn't :/

    I don't quite understand "also not listing for the event and calling directly would avoid the need for this"

@scottgonzalez

This comment has been minimized.

Show comment
Hide comment
@scottgonzalez

scottgonzalez Jan 2, 2014

Member

We handled this scenario in UI by proxying _trigger() in the extension.

Here's an example where we change the data: https://github.com/jquery/jquery-ui/blob/8c9651fe715a0cc4de3004d5d688ee4c8c9edde9/ui/jquery.ui.tabs.js#L1283-L1293

Here's an example where we trigger a different event: https://github.com/jquery/jquery-ui/blob/8c9651fe715a0cc4de3004d5d688ee4c8c9edde9/ui/jquery.ui.tabs.js#L1197-L1204

Member

scottgonzalez commented Jan 2, 2014

We handled this scenario in UI by proxying _trigger() in the extension.

Here's an example where we change the data: https://github.com/jquery/jquery-ui/blob/8c9651fe715a0cc4de3004d5d688ee4c8c9edde9/ui/jquery.ui.tabs.js#L1283-L1293

Here's an example where we trigger a different event: https://github.com/jquery/jquery-ui/blob/8c9651fe715a0cc4de3004d5d688ee4c8c9edde9/ui/jquery.ui.tabs.js#L1197-L1204

@gabrielschulhof

This comment has been minimized.

Show comment
Hide comment
@gabrielschulhof

gabrielschulhof Jan 2, 2014

Contributor

OK. I can use this._widget._trigger(), because I this.element.data( "mobile-listview" ) in this._widget.

Contributor

gabrielschulhof commented Jan 2, 2014

OK. I can use this._widget._trigger(), because I this.element.data( "mobile-listview" ) in this._widget.

@gabrielschulhof

This comment has been minimized.

Show comment
Hide comment
@gabrielschulhof

gabrielschulhof Jan 2, 2014

Contributor

In fact, using this._widget._trigger() avoids having to check whether event.target === this.element, because I'm catching the event at the point of emission. Nice! Thanks @scottgonzalez !

Contributor

gabrielschulhof commented Jan 2, 2014

In fact, using this._widget._trigger() avoids having to check whether event.target === this.element, because I'm catching the event at the point of emission. Nice! Thanks @scottgonzalez !

@arschmitz

This comment has been minimized.

Show comment
Hide comment
@arschmitz

arschmitz Jan 8, 2014

Member

Looks good now squash this down to a single commit and then 👍

Member

arschmitz commented Jan 8, 2014

Looks good now squash this down to a single commit and then 👍

@gabrielschulhof gabrielschulhof deleted the 6679-trigger-listviewbeforefilter branch Jan 9, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment