-
Notifications
You must be signed in to change notification settings - Fork 799
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
Instant Search: Update prototype with overlay design #14550
Conversation
* Add label and button to search box and convert to functional component * Correct lodash import
* Turn statements into sentence fragments * Add appropriate landmark roles for elements * Set portaled widget as a search form * Use ordered list for search results
* [not verified] Add product search result component * [not verified] Refactor renderResult into component * [not verified] Convert SearchResult to stateless functional component * [not verified] Move comments to new shared component * [not verified] Move Tracks out to common SearchResult * [not verified] Request additional fields for product results * Correct field names and remove ratings for now * Basic styling of cards with images * Add price where available * Read result_format from the query string * Use ordered list * Style cleanup * Minor cleanup * Remove resultFormat from component state * SearchResults component cleanup * Standardise class names * Validate result format before using as CSS class * Address review nits
* Add PhotonImage component * Bump default dimensions to 300x300 * Bump asset size limit to 100kb
* Add label and button to search box and convert to functional component * Correct lodash import
* Turn statements into sentence fragments * Add appropriate landmark roles for elements * Set portaled widget as a search form * Use ordered list for search results
* [not verified] Add product search result component * [not verified] Refactor renderResult into component * [not verified] Convert SearchResult to stateless functional component * [not verified] Move comments to new shared component * [not verified] Move Tracks out to common SearchResult * [not verified] Request additional fields for product results * Correct field names and remove ratings for now * Basic styling of cards with images * Add price where available * Read result_format from the query string * Use ordered list * Style cleanup * Minor cleanup * Remove resultFormat from component state * SearchResults component cleanup * Standardise class names * Validate result format before using as CSS class * Address review nits
* Add PhotonImage component * Bump default dimensions to 300x300 * Bump asset size limit to 100kb
… into instant-search-master
* Instant Search: use default values for getting results to avoid duplication * getResults: default to empty object Co-Authored-By: Jason Moon <4044428+jsnmoon@users.noreply.github.com>
A quick note about Jeremy's feedback, we need to remove the test for
Removing it should allow for the tests to pass and we'll be able to approve. |
fea47ea
to
9d1caef
Compare
9d1caef
to
cfc93e2
Compare
Fixed broken tests and updated the whitelist
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing all the feedback 🥇
I'm marking this approved, however I'll rely on @jeherve to see if there is anything special about the WP.com merge as there seems to be a conflict so it can't cleanly create.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not the easiest thing to review, but most of it seems to work at the moment. I have some comments, but nothing blocking in my opinion. This can all be addressed in follow-up PRs.
I am also getting those notices:
PHP Notice: Undefined offset: 1 in wp-content/plugins/jetpack-dev/modules/widgets/search.php on line 643
PHP Notice: Undefined offset: 1 in wp-content/plugins/jetpack-dev/modules/widgets/search.php on line 650
Thank you for your thorough review, @jeherve! :) |
Deployed changes to package and yarn.lock in r202800-wpcom. |
Thanks @zinigor FYI, I will be working on the merge to wpcom early next week. It didn't get auto created. |
That's indeed to be expected right now.
|
Changes proposed in this Pull Request:
Is this a new feature or does it add/remove features to an existing part of Jetpack?
Testing instructions:
Follow these setup instructions to set up Jetpack Instant Search on your site. Please note that the Jetpack Search widget will need to be added to twice: once to your site sidebar/footer and once to the Jetpack Search Sidebar within the search overlay.
Submit a query into your site search input. Ensure that a search overlay appears with your search results.
Enter your site's customizer (
/wp-admin/customize.php
) and navigate to the "Jetpack Search" section. Ensure that changing the settings there take effect on the overlay without a page reload.Try enabling and using filters on the widget in your site's sidebar/footer. Ensure that they behave as expected.
Try enabling and using filters on the widget inside the search overlay. Ensure that they behave as expected.
Proposed changelog entry for your changes: