All search styling #11550
Merged
All search styling #11550
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
# Conflicts: # content/webapp/pages/search/index.tsx
# Conflicts: # content/webapp/pages/search/index.tsx
|
Size Change: +1.05 kB (+0.1%) Total Size: 1.02 MB
ℹ️ View Unchanged
|
davidpmccormick
approved these changes
Feb 3, 2025
rcantin-w
reviewed
Feb 3, 2025
Comment on lines
+369
to
+376
| {(!!( | ||
| catalogueResults.images?.totalResults && | ||
| catalogueResults.images?.totalResults > 0 | ||
| ) || | ||
| !!( | ||
| catalogueResults.works?.totalResults && | ||
| catalogueResults.works?.totalResults > 0 | ||
| )) && ( |
There was a problem hiding this comment.
I think catalogueResults.images?.totalResults > 0 || catalogueResults.works?.totalResults > 0 could be simplified equivalent? (undefined > 0 = false)
There was a problem hiding this comment.
I can't get this to work
There was a problem hiding this comment.
Ok I hadn't tested it so possible! I just thought it was a bit complex to read but it's not hard to understand what it actually does, so np
rcantin-w
reviewed
Feb 3, 2025
Another one to run past Dana is are we limiting the work types being displayed to the first X ones? |
rcantin-w
reviewed
Feb 3, 2025
rcantin-w
reviewed
Feb 3, 2025
Comment on lines
406
to
415
| <NextLink | ||
| {...worksLink( | ||
| { | ||
| query: queryString, | ||
| }, | ||
| `works_all_${pathname}` | ||
| )} | ||
| passHref | ||
| legacyBehavior | ||
| > |
There was a problem hiding this comment.
These links don't work for me locally, there's no href in the element
There was a problem hiding this comment.
hmm, I'm struggling to replicate this
There was a problem hiding this comment.
I pulled the latest, running with run-concurrently and both of first and subsequent loads these links don't work for me 🤔
There was a problem hiding this comment.
sorry, I'm looking at the wrong links!
There was a problem hiding this comment.
tbf I might've commented the wrong one 😅
There was a problem hiding this comment.
I read up about legacyBehavior but didn't think to check the links!
There was a problem hiding this comment.
That'd be the last thing for me, otherwise good to go and very nice to see!
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.
What does this change?
For #11459
Implements styling for the catalogue and image results (designs)
Wide screen:
Narrow screen:
No results:
How to test
View search page will allSearch toggle enabled and compare with the designs)
How can we measure success?
The results match the designs
Have we considered potential risks?
Behind a toggle so now really
I've created a few new components, we may want to move them into there own files/add them to cardigan if we think they will be reused.