Skip to content
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

Allow overriding the default Suggestion class #17177

Open
wants to merge 3 commits into
base: gh-pages
Choose a base branch
from
Open

Allow overriding the default Suggestion class #17177

wants to merge 3 commits into from

Conversation

Fustrate
Copy link

@Fustrate Fustrate commented Mar 2, 2019

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
Copy link
Owner

LeaVerou commented Mar 4, 2019

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
Copy link
Author

Fustrate commented Mar 5, 2019

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
Copy link
Author

Fustrate commented Mar 5, 2019

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
Copy link
Owner

LeaVerou commented Mar 5, 2019

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

@Fustrate
Copy link
Author

Fustrate commented Mar 6, 2019

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
Copy link
Author

Fustrate commented Mar 10, 2019

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.

None yet

3 participants