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

Enhancement: Add renderItemType prop to ContentSearch #162

Merged
merged 3 commits into from
Nov 18, 2022

Conversation

rmorse
Copy link
Contributor

@rmorse rmorse commented Oct 25, 2022

Description of the Change

Adds a callback to allow customisation of the item type in the suggestions list.

Closes #160

How to test the Change

Add the renderItemType prop to the ContentSearch component.

<ContentSearch
	{ ...props }
	renderItemType={ ( suggestion ) => {
		return suggestion.subtype;
	} }
/>

Changelog Entry

Added - New feature - customise the item type rendered in the ContentSearch component.

Credits

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

I had trouble creating the dev environment so couldn't spin up the tests.

@fabiankaegy
Copy link
Member

Thank you for working on this @rmorse 🚀

I do like this enhancement quite a bit! There is one thing that I want to think through before merging this in though. I'm wondering whether it would be a better more flexible/maintainable API to provide a way for consumers of this component to pass in their own suggestionTemplate.

I've seen that pattern get used by various libraries like for example the alphagov-accessible-autocomplete

That way we could ensure that we can support all kinds of customizations.

The reason why I am suggesting that we discuss this is that I don't want to end up in a place where we need 5 different functions to modify each small little piece of the suggestion individually because it can get very messy to maintain.

I'm very open to suggestions though and maybe in this case one doesn't mean the other isn't also useful since changing the type label is a more common action and I could see how having to rebuild the entire suggestion markup just to modify this one piece can be overkill.

I'd love to get @Antonio-Laguna to chime in here aswell how he thinks we could proceed with this.

@rmorse
Copy link
Contributor Author

rmorse commented Oct 25, 2022

Thanks Fabian, I just rolled with this for now as it was something I needed (immediately!) and thought I'd share it in case it's something you wanted to add in.

Totally understand the need to future proof this and not add too many props.

I do like the idea of the suggestionTemplate, sounds like a great idea for complete customisation

Copy link
Member

@Antonio-Laguna Antonio-Laguna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rmorse thanks a ton for working on this!

As @fabiankaegy hints, I think the way to go moving forward is to have template parts as props. This is something I've been pondering in general for the blocks as I've seen the need for complex ones such as ContentPicker.

I've reviewed this (nice work!) and I do see the value of this prop vs the more cumbersome path of having to write your own component which is a bit overkill. When we change to templates as props this will just be a prop too and you can do (or not do) as you want with the prop.

👍 from me!

@@ -87,6 +100,7 @@ SearchItem.defaultProps = {
id: '',
searchTerm: '',
isSelected: false,
renderType: null,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rmorse thoughts on putting the logic you added on L53 here as a sensible default? That way the logic from L52:L57 can be simplified as renderType would always have a function (unless the prop has a value type it shouldn't but that's ok).

If you think it's right you should also update the default value on content-search/index.js so it's undefined instead of null

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Antonio-Laguna do you mean something like this?

SearchItem.defaultProps = {
	...
	renderType: ( suggestion ) => {
		return suggestion.type === 'post_tag' ? 'tag' : suggestion.type;
	},
	...
}

Agreed, this looks cleaner to me, happy to add in.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly that!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great stuff, will get this tidied up tomorrow

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Antonio-Laguna update just landed

Add default function for renderType
Update documentation
@fabiankaegy fabiankaegy merged commit 2855cc3 into 10up:develop Nov 18, 2022
@fabiankaegy
Copy link
Member

Thanks again for the update here :) This is a great addition!

@rmorse rmorse deleted the ContentSearch branch November 18, 2022 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Customize the ContentSearch item type label
3 participants