Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#9195 closed bug (notabug)

undocumented hack used in example

Reported by: penguin_brian Owned by: penguin_brian
Priority: minor Milestone: none
Component: ui.autocomplete Version: 1.10.2
Keywords: Cc:
Blocked by: Blocking:

Description

http://jqueryui.com/autocomplete/#custom-data

has:

.data( "ui-autocomplete" )._renderItem

However this is not documented anywhere on the API documentation page:

http://api.jqueryui.com/autocomplete/

And as such, looks like an undocumented hack. It should be possible to override the renderItem functionality through a parameter to .autocomplete().

This also means it would be possible to configure this at a much more convenient time during the load cycle. At the moment it doesn't appear possible to access .data( "ui-autocomplete" ) until after the element has been added to the document which can be inconvenient.

Thanks

Change History (15)

comment:1 Changed 7 years ago by tj.vantoll

Owner: set to penguin_brian
Status: newpending

You can override _renderItem for the widget by using $.ui.autocomplete.prototype._renderItem = function( ui, item) { // Your implementation

We don't document methods that are intended for private use by the widgets. The source option (http://api.jqueryui.com/autocomplete/#option-source) does link to Scott's HTML extension (https://github.com/scottgonzalez/jquery-ui-extensions/blob/master/autocomplete/jquery.ui.autocomplete.html.js) if you want to use HTML in the displayed options.

What is your use case that you want to override, _renderItem? Are you just looking to display HTML?

comment:2 Changed 7 years ago by Scott González

Resolution: notabug
Status: pendingclosed

_renderItem() is public API, it's just not part of the API exposed via the standard interface. See https://github.com/jquery/api.jqueryui.com/issues/20

comment:3 Changed 7 years ago by penguin_brian

My use case is that I want to render HTML, like per the example. Which is documented on the demo page, despite the fact it isn't part of the standard interface.

I really think this functionality should be exposed by the standard interface.

As an example of the problems this causes, it is not possible to do:

$("<input ...>")
.autocomplete({...})
.data( "ui-autocomplete" )._renderItem(..)
.appendTo(form)

Because data( "ui-autocomplete" ) doesn't exist at this point in time. Instead you have to resort to further kludges, to delay the initialization until after the form is added to the page.

I would like to be able to do:

$("<input ...>")
.autocomplete({...
    renderItem = function(...) { ... }
})
.appendTo(form)

And just have it work without any kludges.

comment:4 Changed 7 years ago by penguin_brian

Just another note, the .data("ui-autocomplete"), which is used by the demo, was .data("autocomplete") for earlier versions of jquery, so this looks like something that can be changed without warning. That is it is not a stable public API that should be used the way the demo is using it.

I think perhaps the demo code should class a new widget inherited from ui.autocomplete that overrides _renderItem in a clear fashion without resorting to using .data.

Would you be willing to accept a new version of http://jqueryui.com/autocomplete/#custom-data that does this?

comment:5 Changed 7 years ago by penguin_brian

Now I investigated, was easier then I thought:

    $.widget('ui.htmlautocomplete', $.ui.autocomplete, {
      _renderItem: function( ul, item ) {
          return $( "<li>" )
          .append( "<a>" + item.label + "<br>" + item.desc + "</a>" )
          .appendTo( ul );
       }
    });

This goes at the top of the JavaScript, and lines 66-70 and should be deleted. Then on line 50 (before inserting the above code), change the autocomplete reference to htmlautocomplete.

(actually because this is JavaScript inside HTML, some of those characters really should be quoted, but I left them as is because I don't want to make to many changes.)

No hacking with .data(xxx) required.

comment:6 Changed 7 years ago by tj.vantoll

The old naming convention used within .data was deprecated in 1.9 with a clear indication that it was going away in 1.10: http://jqueryui.com/upgrade-guide/1.9/#changed-naming-convention-for-data-keys. It was again mentioned in the 1.10 upgrade guide: http://jqueryui.com/upgrade-guide/1.10/#removed-data-fallbacks-for-widget-names.

The preferred way of handling this will be the instance method which has been added to the widget factory and will be landing in 1.11. This demo has already been updated https://github.com/jquery/jquery-ui/commit/38c7b1ca814c59c48522cda6cfdf054c61773959.

comment:7 Changed 7 years ago by Scott González

TJ covered the important parts, but I want to add a few notes:

You're explanation of why the following fails is completely wrong:

` $("<input ...>") .autocomplete({...}) .data( "ui-autocomplete" )._renderItem(..) .appendTo(form) `

The instance is definitely stored in data at that point. You shouldn't be invoking _renderItem() here, you should be overriding it. You can't chain .appendTo() like that because you're moved to the instance.

If you'd like to be able to set renderItem as an option, write an extension. It'll be less than 10 lines, and will blissfully allow you to do completely incorrect things without noticing at first.

For example, your implementation is htmlautocomplete is incorrect. Supporting HTML is *NOT* as simple as just changing the rendering. You also need to change the search logic because you don't want to match on HTML.

comment:8 Changed 7 years ago by penguin_brian

Oops, looks like I did stuff up the example in comment:3. Retry:

    var i = $("<input type='hidden' />")
        .autocomplete({})
    i.data("ui-autocomplete")._renderItem = function _renderItemHTML(ul, item) { };

If I put this code in my project, it dies with 'Uncaught TypeError: Cannot set property '_renderItem' of null'. If I put the same code at the top of the demo code, it works. So I may have to investigate more what is going on here.

The demo does not change the search logic, so if that it true, then the demo is broken. Actually I think it only searches item.label by default, and the intended idea is that item.label not contain HTML, so it should be ok (so it should really be quoted somehow, not sure how to do this though for this code).

Last edited 7 years ago by penguin_brian (previous) (diff)

comment:9 Changed 7 years ago by mikesherov

penguin_brain, thanks for the input and investigative work, but please consider taking this discussion to the forums or stackoverflow, as it no longer seems to be discussing a bug but rather your implementation of a feature.

comment:10 Changed 7 years ago by penguin_brian

Some of the things said here are clearly incorrect, this seems like the best place to address those errors. It does seem like jquery-ui team really aren't interested in my bug reports, I feel every thing I have said has been attacked without trying to address the root problems. So rather then trying to work together on the issue, I get defensive.

If there are any problems in the way I am using autocomplete, it is because the demo is broken, because I have copied the same techniques used in the demo. However you don't seem to accept that there is anything wrong with the demo as is currently available on the website.

Anyway, I will comply, this will be my last post on the issue.

comment:11 Changed 7 years ago by mikesherov

To be clear, no one is attacking you. The bug you reported is being addressed as a documentation issue. The place to discuss specific implementation questions and concerns is the forum, not the bug tracker. My response to you was polite and straightforward.

comment:12 Changed 7 years ago by Scott González

If I put this code in my project, it dies with 'Uncaught TypeError: Cannot set property '_renderItem' of null'. If I put the same code at the top of the demo code, it works. So I may have to investigate more what is going on here.

Sounds like a bug in your code, since it clearly works in the demo. Without a live test case it's impossible for us to provide any help. Please stop repeating your arguments when there is clear evidence to the contrary. Provide a reduced test case that shows an actual problem or we will just continue to repeat ourselves (actually, we'll just stop replying).

The demo does not change the search logic, so if that it true, then the demo is broken.

No, the demo doesn't claim to be implementing an HTML label. The label is the actual content, with no markup. The rendering is custom, but expects the label from the data source to be plain text, so searching works exactly as expected. I was pointing out that if you're trying to treat the label as containing markup, then you're breaking the built-in search functionality.

comment:13 Changed 7 years ago by penguin_brian

I just noticed, this discussion seems to have wondered somewhat off-topic. Mistakes made in my examples didn't help (trying to do too many things at once). So will try to get back on topic.

In comment:1 I was told "We don't document methods that are intended for private use by the widgets" which implies that the demo should not be using a method that is intended for private use by the widgets. Then in comment:2 I am told "_renderItem() is public API, it's just not part of the API exposed via the standard interface." which seems to conflict with the first statement. Is it public/private or what? If it is part of the public API, why is it not documented in the standard API documentation?

As such I find these sorts of statements ... confusing. There are other statements throughout this ticket that I found equally confusing.

It wasn't until comment:6 that somebody thought to mention that the existing data("ui-autocomplete") is changing, to something that looks a lot better.

I am responding here, because this is the logical place to put a response. It doesn't make sense to respond somewhere else, where no one reading the ticket will be able to find it.

I also think there is a concern with making the API stable (1.9 uses data("autocomplete"), 1.10 uses data("ai-autocomplete), 1.11 uses autocomplete( "instance" )), which makes it hard to write libraries that support all versions. Maybe nothing that can be done about this now, hopefully the new method will remain stable. Which like I said, it does seem a much better way of doing it.

For the record (will remain very brief), I have also worked out the problems using .data(...) with https://github.com/crucialfelix/django-ajax-selects, and created a fork to resolve some of its problems https://github.com/brianmay/django-ajax-selects/tree/js - however we were getting lead astray with this discussion, that wasn't the point of this bug report.

Thanks

comment:14 Changed 7 years ago by penguin_brian

Responding to comment:11, should also clarify, I didn't mean anyone was attacking me personally. I was thrown way of target when somebody responded to my comment:3 in comment:7. Nobody even seemed to notice my comment:5.

I think my solution in comment:5 might be the best solution, and I think it will work across all versions.

comment:15 Changed 7 years ago by Scott González

Is it public/private or what? If it is part of the public API, why is it not documented in the standard API documentation?

It's public. TJ wasn't aware because of the fact that it's not documented. Go read comment:2 again.

It wasn't until comment:6 that somebody thought to mention that the existing data("ui-autocomplete") is changing, to something that looks a lot better.

There was no need to mention this sooner. It's not related to the "bug" you were reporting in this ticket.

I am responding here, because this is the logical place to put a response. It doesn't make sense to respond somewhere else, where no one reading the ticket will be able to find it.

You could easily create a thread on the forum and then add a link to it in a comment.

I also think there is a concern with making the API stable (1.9 uses data("autocomplete"), 1.10 uses data("ai-autocomplete), 1.11 uses autocomplete( "instance" )), which makes it hard to write libraries that support all versions. Maybe nothing that can be done about this now, hopefully the new method will remain stable. Which like I said, it does seem a much better way of doing it.

Again, not related to this ticket. Also, this has already been solved with the instance() method. There is nothing else to do here.

...however we were getting lead astray with this discussion, that wasn't the point of this bug report.

Which is why we didn't want this to continue...

I think my solution in comment:5 might be the best solution, and I think it will work across all versions.

There's a difference between writing a one-off customization and writing a new widget. The custom data demo is a one-off. There's one place where we're using a custom data source with custom rendering. The categories demo is a good example of a generic application in which a new widget is created.

The bottom line is this ticket is about use of an "undocumented hack". This turns out not to be true, as pointed out as early as comment:2.

Note: See TracTickets for help on using tickets.