Skip to content

Allow overriding the default Suggestion class - #17177

Open
Fustrate wants to merge 3 commits into
nudeui:gh-pagesfrom
Fustrate:gh-pages
Open

Allow overriding the default Suggestion class#17177
Fustrate wants to merge 3 commits into
nudeui:gh-pagesfrom
Fustrate:gh-pages

Conversation

@Fustrate

@Fustrate Fustrate commented Mar 2, 2019

Copy link
Copy Markdown

I'm using Awesomplete for 8 separate dropdowns that need custom li html, and being able to use a custom Suggestion implementation for each item type would really help. Unfortunately, the creation of a Suggestion is hard-coded, so here's a pull request to make it a config option like _.DATA or _.ITEM.

I see there's been a previous pull request about spreading list props (#17001) and I think this would help in that area without causing any backwards-compatibility problems or over-complication for people who don't need this feature.

@LeaVerou

LeaVerou commented Mar 4, 2019

Copy link
Copy Markdown
Collaborator

Thanks! I love PRs that also add documentation!

Why the wrapper function however? Why not have the option directly point to the class to use?

@Fustrate

Fustrate commented Mar 5, 2019

Copy link
Copy Markdown
Author

I guess that's a remnant of my roundabout way of getting here. I was doing some processing before actually creating the suggestion, but now that I look through my code that's using this PR, it's all just variations of data => new WhateverSuggestion(data). I'll make the change to remove the wrapper function.

Signed-off-by: Steven Hoffman <git@fustrate.com>
@Fustrate

Fustrate commented Mar 5, 2019

Copy link
Copy Markdown
Author

I just remembered why I did it that way. I have a couple autocompletes that I haven't switched to this branch which use 2 different suggestion types in one field - PhoneNumberSuggestion/EmailAddressSuggestion to fill out a "Contact At" input, and VendorSuggestion/ClientSuggestion for a contact form. Without a way to check what type of datum is being passed in from one of multiple sources, it can't determine the right class to use.

I'm going to revert back to the original way, convert those fields to make sure it works, and then reopen this PR.

@Fustrate Fustrate closed this Mar 5, 2019
@LeaVerou

LeaVerou commented Mar 5, 2019

Copy link
Copy Markdown
Collaborator

I still don't understand. Wouldn't e.g. instanceof VendorSuggestion work?

@Fustrate

Fustrate commented Mar 6, 2019

Copy link
Copy Markdown
Author

That would work later on, but if the default setting only returns a class, it can't know which Suggestion subclass to return.

I'll reopen this PR in the next couple days - I'm currently in the middle of converting a codebase from CoffeeScript to ES6, and I'm just about to the part where dynamic classes will come into play.

Signed-off-by: Steven Hoffman <git@fustrate.com>
@Fustrate

Fustrate commented Mar 10, 2019

Copy link
Copy Markdown
Author

Okay, I have no idea why it's working, but with the current no-wrapper PR and a function that returns an instantiated object, it does work. I'm just going to reopen the PR and not think too hard about why new is being used twice with no ill effects. I've also added a few tests to reassure myself, though they seem a bit fragile to me because Suggestion is private for the first test, and I hard coded one of the list values in the third test.

@Fustrate Fustrate reopened this Mar 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants