Page MenuHomePhabricator
Create Task
Maniphest T86851

LookupInputWidget is terrible and also sucks
Closed, ResolvedPublic
Actions

Description

Brain dump:

  • The constructor called like OO.ui.LookupInputWidget.call( this, this, config ) is stupid – fixed with https://gerrit.wikimedia.org/r/184828
  • The name is wrong – should be InputLookupWidget maybe, or just LookupWidget? It is not a subclass of InputWidget.
  • The methods #getLookupCacheItemFromData and #getLookupMenuItemsFromData actually accept different data as input – in fact, one takes the output of the other (see example: T85467#975078)
  • The #onLookupMenuItemChoose method has to be reimplemented in every child, while in 90% of cases it should just set the value of textbox to the chosen suggestion (pointed out by @Ricordisamoa)

Details

Related Changes in Gerrit:
SubjectRepoBranchLines +/-
[BREAKING CHANGE] Remove deprecated LookupInputWidgetoojs/uimaster+0 -331
Replace old&busted LookupInputWidget with new&hot LookupElementoojs/uimaster+415 -0
Revert "[BREAKING CHANGE] LookupInputWidget: Remove the silly 'input' parameter"oojs/uimaster+11 -8
Customize query in gerrit

Related Objects
Search...

StatusSubtypeAssignedTask
 ResolvedmatmarexT85467 No demo for LookupElement
 ResolvedmatmarexT86851 LookupInputWidget is terrible and also sucks

Event Timeline

matmarex created this task.Jan 14 2015, 11:13 PM
matmarex raised the priority of this task from to Medium.
matmarex updated the task description. (Show Details)
matmarex added a project: OOUI.
matmarex added subscribers: matmarex, Osnard, Ricordisamoa and 4 others.

I would really like to fix these, but @Jdforrester-WMF is already mad at me for making https://gerrit.wikimedia.org/r/184828 breaking ;), so here's a proposal: we revert that, we deprecate LookupInputWidget and generally cast it into oblivion, we create a new pretty InputLookupWidget that fixes the issues above, and we replace uses of old one with the new one (the only user currently is VisualEditor and @Jdlrobson's unmerged MobileFrontend change).

Thoughts?

matmarex mentioned this in T85467: No demo for LookupElement.Jan 14 2015, 11:16 PM
matmarex added subscribers: • TrevorParscal, Catrope.
Krenair subscribed.Jan 14 2015, 11:20 PM
Ricordisamoa added a comment.Jan 15 2015, 12:08 AM

Also, onLookupMenuItemChoose is not a standard method, it has to be bound with this.lookupMenu.connect().

Are there any use cases of LookupInputWidget which don't involve inheriting from TextInputWidget?

matmarex added a comment.Jan 15 2015, 12:13 AM
In T86851#977887, @Ricordisamoa wrote:

Are there any use cases of LookupInputWidget which don't involve inheriting from TextInputWidget?

Yes, VisualEditor's ve.ui.MWLinkTargetInputWidget (which mixins a LookupInputWidget) inherits from ve.ui.LinkTargetInputWidget (and that inherits from TextInputWidget).

matmarex added a comment.Jan 15 2015, 12:45 AM

Talked with @TrevorParscal and we probably want to call it… LookupElement. Fits better with other mixins' names.

matmarex claimed this task.Jan 15 2015, 1:00 AM
gerritbot added a project: Patch-For-Review.Jan 15 2015, 1:25 AM
gerritbot subscribed.

Change 185110 had a related patch set uploaded (by Bartosz Dziewoński):
Revert "[BREAKING CHANGE] LookupInputWidget: Remove the silly 'input' parameter"

https://gerrit.wikimedia.org/r/185110

Patch-For-Review

matmarex raised the priority of this task from Medium to High.Jan 15 2015, 1:30 AM
matmarex added a parent task: T85467: No demo for LookupElement.
gerritbot added a comment.Jan 15 2015, 1:44 AM

Change 185110 merged by jenkins-bot:
Revert "[BREAKING CHANGE] LookupInputWidget: Remove the silly 'input' parameter"

https://gerrit.wikimedia.org/r/185110

matmarex removed a project: Patch-For-Review.Jan 15 2015, 2:19 AM
matmarex set Security to None.
Jdforrester-WMF moved this task from Backlog to Doing on the OOUI board.Jan 15 2015, 2:21 AM
matmarex added a comment.Jan 15 2015, 7:43 PM

Further issues:

  • The lookup menu doesn't have z-index (?) high enough to display on top of other widgets, unlike the one in DropdownWidget and its kin
    pasted_file (707×297 px, 13 KB)
  • Should probably call #isValid before triggering the lookup queries?
Ricordisamoa added a comment.Jan 15 2015, 8:38 PM
In T86851#980120, @matmarex wrote:

Further issues:

  • The lookup menu doesn't have z-index (?) high enough to display on top of other widgets, unlike the one in DropdownWidget and its kin
    pasted_file (707×297 px, 13 KB)

This issue is still a blocker for my use cases.

  • Should probably call #isValid before triggering the lookup queries?

I don't think so, for example the validity regex could be /\w{3,}/ and the user might try to type a single letter to get suggestions.

matmarex added a comment.Jan 15 2015, 9:27 PM

I don't think so, for example the validity regex could be /\w{3,}/ and the user might try to type a single letter to get suggestions.

Hmm, okay.

gerritbot added a project: Patch-For-Review.Jan 15 2015, 9:31 PM

Change 185254 had a related patch set uploaded (by Bartosz Dziewoński):
Replace old&busted LookupInputWidget with new&hot LookupElement

https://gerrit.wikimedia.org/r/185254

Patch-For-Review

matmarex added a comment.Jan 15 2015, 9:32 PM

Thank you for the really useful comments! :D

gerritbot added a comment.Jan 15 2015, 9:36 PM

Change 185257 had a related patch set uploaded (by Bartosz Dziewoński):
[BREAKING CHANGE] Remove deprecated LookupInputWidget

https://gerrit.wikimedia.org/r/185257

Patch-For-Review

gerritbot added a comment.Jan 15 2015, 10:57 PM

Change 185254 merged by jenkins-bot:
Replace old&busted LookupInputWidget with new&hot LookupElement

https://gerrit.wikimedia.org/r/185254

matmarex closed this task as Resolved.Jan 15 2015, 11:36 PM
Jdforrester-WMF added a project: WMF-deploy-2015-02-04_(1.25wmf16).Jan 16 2015, 12:04 AM
Ricordisamoa mentioned this in T85394: Use OOUI suggestions/autocompletion components only (instead of jquery.suggestions, jquery.ui.autocomplete).Jan 16 2015, 12:10 AM
Jdforrester-WMF removed a project: Patch-For-Review.Jan 30 2015, 6:23 PM
gerritbot added a comment.Feb 27 2015, 6:45 PM

Change 185257 merged by jenkins-bot:
[BREAKING CHANGE] Remove deprecated LookupInputWidget

https://gerrit.wikimedia.org/r/185257

matmarex mentioned this in rGOJU03f17555f885: Revert "[BREAKING CHANGE] LookupInputWidget: Remove the silly 'input' parameter".Mar 3 2015, 7:49 PM
matmarex mentioned this in rGOJUac3d0d2e429f: Replace old&busted LookupInputWidget with new&hot LookupElement.
Jdforrester-WMF mentioned this in rGOJUeb5210389ad2: [BREAKING CHANGE] Remove deprecated LookupInputWidget.Mar 3 2015, 7:51 PM
Jdforrester-WMF moved this task from Doing to Reviewing on the OOUI board.Mar 26 2015, 8:57 PM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits