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

Add searchform partial and function to replace default WordPress functionality #2090

Merged
merged 6 commits into from Aug 9, 2018
Merged

Conversation

MWDelaney
Copy link
Member

Inspired by this thread:
https://discourse.roots.io/t/custom-search-form/9079

The WordPress searchform, and the ability to customize it, is basic WP functionality that Sage unintentionally breaks. This PR predictably replaces the WP default template with a Blade partial, much like the the comments partials we already ship, letting users use get_search_form() as normal.

@@ -1,7 +1,7 @@
<form role="search" method="get" class="search-form" action="{{ esc_url( home_url( '/' ) ) }}">
<form role="search" method="get" class="search-form" action="{{ esc_url(home_url( '/') ) }}">
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

👀

Copy link

Choose a reason for hiding this comment

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

See commit d93964b -- this was already done

This removes all logic from the search form Blade template, and extracts
it to the "Controller" level via Sage's filter system. It also adds a
global 'app' filter to hook into, so that the search form data will be
served on every page.
Copy link
Member Author

@MWDelaney MWDelaney left a comment

Choose a reason for hiding this comment

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

is "sf" descriptive enough?

@alwaysblank
Copy link
Member

I'm not super attached to it, I just cut it down because I felt like search_form_ was going to lead to some very long variables names.

Copy link

@garrettw garrettw left a comment

Choose a reason for hiding this comment

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

Good call!

Copy link
Member Author

@MWDelaney MWDelaney left a comment

Choose a reason for hiding this comment

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

Looks good here.

@darrenjacoby
Copy link

Could just use search ?

@alwaysblank
Copy link
Member

IMO search is insufficiently specific since there are other contexts (i.e. results, etc) where you could feasibly be using $search_ and end up with name collisions.

@darrenjacoby
Copy link

true

@MWDelaney
Copy link
Member Author

So yay?

@alwaysblank
Copy link
Member

🚀

@mmirus
Copy link
Contributor

mmirus commented Aug 7, 2018

Would it make more sense to put the $sf_* stuff in the app controller? Out of the box, Sage seems slightly more oriented towards using that approach. Though I think the docs introduce filters as the easy way, the code itself emphasizes the controller more, and I think most discussion on Discourse focuses on them too.

No strong feelings, just raising the question. Other than that, LGTM. Thanks all!

@alwaysblank
Copy link
Member

My reason for the filter approach is that it's the "native" approach—Controller is a 3rd party package. IMO if we offer a "native" way to do a thing then our most basic implementation should use that implementation, not depend on a 3rd party package.

@mmirus
Copy link
Contributor

mmirus commented Aug 7, 2018

That makes sense to me. TBH I thought it was odd that Sage uses the controller out of the box and not the data filters (to the extent it uses either) for the same reason. Could just be because App::title() wouldn't fit in a data filter.

app/filters.php Outdated
$data['sf_placeholder'] = esc_attr_x('Search &hellip;', 'placeholder', 'sage');
$data['sf_current_query'] = get_search_query();
$data['sf_submit_text'] = esc_attr_x('Search', 'submit button', 'sage');
return $data;
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

return $data + [
    'sf_action' => esc_url(home_url('/')),
    // etc...
];

That will preserve sf_* key-value pairs that are already present in the $data array and only merge them in if they don't already exist.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should be all set

@garrettw
Copy link

garrettw commented Aug 8, 2018

Need to fix indenting to pass phpcs. Line 87 has 1 space, should be 0; the rest should use 4-space indents rather than 2.

@MWDelaney
Copy link
Member Author

So, yay?

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

8 participants