V3.513 search patterns #519
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- added typehead style mockup - now handles proper theme color application
designerbrent
suggested changes
Jan 28, 2021
There was a problem hiding this comment.
A few comments after I reviewed it:
- We need to have the content be locked on the grid for the search results. Do we need to switch to using the
media-block? I'd rather not add a new block if we can avoid it. - The other thing that appears to be missing is the "This Site" or "Other Site" delineators in the suggestions (below). These need to be optional ways of sorting the results in the search suggestions:
|
@designerbrent This PR is ready for review! It's been so long since I've worked on this project, so I can't recall what a staging link would be, but here are the links locally for review:
|
|
@kelscahill will the changes you made with the |
|
@designerbrent There shouldn't be any breaking changes with the css and js. |
designerbrent
previously approved these changes
Mar 18, 2021
|
@kelscahill With that, I'll approve it but I will merge it next week. |
designerbrent
approved these changes
Apr 19, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@designerbrent
There's a couple of things to discuss here before merging. The main one is that in the design, the way it is shown in the layout, it is implied that there should be an inner grid of sorts on each search result to get the layout as shown. Right now we are running this through the basic content-block.twig, which has no grid structure, but rather a float on the image for content alignment. The float is why the search results rows don't have space beneath them - because I am trying to clear the float, but because of the float in the first place, it is making the flow irregular. To avoid this we can either create a new search content block pattern with the right structure, or revise the content block pattern to have a grid structure.
There are a few more items that have been adjusted to match the revised design (spacing / layout) that I am sure you will notice as well. When you have a had a chance to review, let me know, as I am sure there will be some further adjustments to bring this to place where you are happy with it. Thanks and I am moving on to the event patterns now.