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

feat: Add o-autocomplete configurable highlighting #1658

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

andygout
Copy link
Contributor

Describe your changes

This PR applies changes so that custom suggestionTemplate callbacks provided as an option when instantiating an instance of oAutocomplete will now receive two additional arguments:

  • highlightSuggestion() — as declared here
  • isMatchHighlighted — a boolean to determine whether matching or non-matching characters should be highlighted, which in turn is passed as an argument to the highlightSuggestion() function within a custom suggestionTemplate() function (that has been passed as an option when instantiating an instance of oAutocomplete) or within the native suggestionTemplate() function

In the scenario of a custom suggestionTemplate, I'm not sure if it's a code smell or anti-pattern that the isMatchHighlighted value is passed at instantiation and then reappears via the suggestionTemplate() callback where there is technically no compulsion to use that value as intended, i.e. given suggestionTemplate() is custom, the isMatchHighlighted value could be declared within that function, making its presence as a parameter redundant. However, it seems far neater to declare up-front at time of instantiation all the configuration that dictates how you would like the component to behave.

I would also a prefer a better variable name than isMatchHighlighted — set to true it makes sense, though when set to false it implies that it will simply not highlight matched characters (and by inference, that it will highlight nothing at all) rather than expressing that it will highlight the characters that are not a match. Naming is hard…

Scenario 1

Custom suggestionTemplate with highlights applied to options where it matches query

  • suggestionTemplate option is provided
  • isMatchHighlighted option is provided with a value of true
function suggestionTemplate (option, query, highlightSuggestion, isMatchHighlighted) {
	
}

new oAutocomplete(oAutocompleteElement, {
	suggestionTemplate,
	isMatchHighlighted: true,});

1


Scenario 2

Custom suggestionTemplate with highlights applied to options where it does NOT match query

  • suggestionTemplate option is provided
  • isMatchHighlighted option is NOT provided (or is provided with a value of false)
function suggestionTemplate (option, query, highlightSuggestion, isMatchHighlighted) {
	
}

new oAutocomplete(oAutocompleteElement, {
	suggestionTemplate,});

OR

function suggestionTemplate (option, query, highlightSuggestion, isMatchHighlighted) {
	
}

new oAutocomplete(oAutocompleteElement, {
	suggestionTemplate,
	isMatchHighlighted: false,});

2


Scenario 3

o-autocomplete native suggestionTemplate with highlights applied to options where it matches query

  • suggestionTemplate option is NOT provided
  • isMatchHighlighted option is provided with a value of true
new oAutocomplete(oAutocompleteElement, {
	isMatchHighlighted: true,});

3


Scenario 4

o-autocomplete native suggestionTemplate with highlights applied to options where it does NOT match query

  • suggestionTemplate option is NOT provided
  • isMatchHighlighted option is NOT provided (or is provided with a value of false)
new oAutocomplete(oAutocompleteElement, {});

OR

new oAutocomplete(oAutocompleteElement, {
	isMatchHighlighted: false,});

4


Issue ticket number and link

Link to Figma designs

Checklist before requesting a review

  • I have applied percy label for o-[COMPONENT] or chromatic label for o3-[COMPONENT] on my PR before merging and after review. Find more details in CONTRIBUTING.md
  • If it is a new feature, I have added thorough tests.
  • I have updated relevant docs.
  • I have updated relevant env variables in Doppler.

@notlee notlee temporarily deployed to origami-webs-feat-add-o-jbempv May 20, 2024 22:07 Inactive
@frshwtr
Copy link
Contributor

frshwtr commented Jun 19, 2024

Thanks for PR, sorry for such a late review :(

I would also a prefer a better variable name than isMatchHighlighted — set to true it makes sense, though when set to false it implies that it will simply not highlight matched characters (and by inference, that it will highlight nothing at all) rather than expressing that it will highlight the characters that are not a match. Naming is hard…

I think this in itself is leading me to think the decision on what to highlight should be delegated to inside the suggestionTemplate function. This may help reduce confusion in its name and ensure; if there is custom behaviour, then the author of the function must decide if they want to highlight within that function.

Alternatively, renaming to invertHighlighting may also be an option. Though I think this may be just as unclear 😧

@andygout
Copy link
Contributor Author

No worries at all 🙂

I think this in itself is leading me to think the decision on what to highlight should be delegated to inside the suggestionTemplate function.

Would this prevent Scenario 3 described above? I.e. the consumer only wants to invert the highlighting but does not want to create a custom suggestionTemplate function to do so.

If I've understood that correctly then it feels like renaming the variable would be the better option to retain the flexibility of that scenario.

I don't think invertHighlighting is too bad, though it does depend on the consumer first identifying what the default is to understand what the inverse would be.

The alternatives I can suggest (and which are essentially equivalent to one another) are:

  • isHighlightCorrelativeToMatch
  • isHighlightCorrespondentToMatch
  • isHighlightCorrespondingToMatch
  • or a variation on this theme

The adjustment of wording means that it now implies that the highlight is always present, with the remaining words determining how that highlighting is applied.

How do you feel about these?

@frshwtr
Copy link
Contributor

frshwtr commented Jun 24, 2024

Would this prevent Scenario 3 described above? I.e. the consumer only wants to invert the highlighting but does not want to create a custom suggestionTemplate function to do so.

We could create this as a standard function that gets exported as part of the library also.

How do you feel about these?

isHighlightCorrespondingToMatch is the clearest to me out of these three.

I think we can go with the option to keep the boolean in place :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Backlog
Development

Successfully merging this pull request may close these issues.

3 participants