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

Added Search Terms for Searchable Pages #42

Merged
merged 2 commits into from
May 31, 2024
Merged

Conversation

aletail
Copy link
Member

@aletail aletail commented May 29, 2024

https://werkbotstudios.teamwork.com/app/tasks/35150248

Summary

Added Search Terms for Searchable Pages

Testing Steps

Issues/Concerns

  • I decided to only have these search terms on the pages, so they reside in one spot. It is flexible enough that we can change this in the future, but I think this should work for most use cases

Git Flow

  • DO NOT delete "release/*" or "hotfix/*" branches after merging a PR. These are used to publish the next release, and they are deleted automatically.
  • "Squash and merge" is good on "feature/*" into "develop"
  • "Create a merge commit" is good on "release/*" or "hotfix/*" into "main"

@aletail aletail requested a review from tiller1010 May 29, 2024 17:41
@aletail aletail marked this pull request as ready for review May 29, 2024 17:41
Copy link
Member

@tiller1010 tiller1010 left a comment

Choose a reason for hiding this comment

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

I decided to only have these search terms on the pages, so they reside in one spot. It is flexible enough that we can change this in the future, but I think this should work for most use cases

Approving, but I left a note about using settingsFields

* @param FieldList $fields
* @return void
**/
public function updateSettingsFields(FieldList $fields)
Copy link
Member

Choose a reason for hiding this comment

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

This being under settings makes sense for pages, but for other searchable dataobjects, like articles and content layouts, they do not have settings fields.

Copy link
Member Author

@aletail aletail May 30, 2024

Choose a reason for hiding this comment

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

That's a good point. I did not consider Blog articles...

  • I was trying to avoid adding to the CMS fields, simply to not clutter it up any more
  • Layouts I am fine without them noy having the option, as they are not searched directly as in they reference a page
  • Articles though...they should have the setting somewhere

Copy link
Member Author

Choose a reason for hiding this comment

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

@tiller1010 Take a look at this again, I updated so the search terms are added to all objects

Copy link
Member Author

Choose a reason for hiding this comment

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

I forgot to mention the content layout, blog, etc....need their getIndexQuery functions updated to include search terms. I figure I would do that after this is merged and released

SiteTree objects will have it added to their Settings tab. DataObjects will have it added to their main tab
@aletail aletail requested a review from tiller1010 May 30, 2024 16:14
Copy link
Member

@tiller1010 tiller1010 left a comment

Choose a reason for hiding this comment

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

Nice!

@aletail aletail merged commit 90cf0ed into develop May 31, 2024
1 check failed
@aletail aletail deleted the feature/search-terms branch May 31, 2024 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants