-
Notifications
You must be signed in to change notification settings - Fork 1
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
DS-734: Add alumni events to federated search #936
Conversation
…i into DS-734/events-federated
I'll look into the failing tests tomorrow. |
@sherakama Part 1 reviewed. Buttons are buttoning, modals are modaling, searches are searching, focus traps are trapping focus. 👍 One (very) subjective suggestion: when it auto scrolls to the search input on the search page it feels a little cramped at the top of the window. We could give it a little breathing room by tossing a top scroll margin on it. |
@sherakama Part 2: mobile seems good. 👍 |
@sherakama Part 3. Ran into a few issues. Everything not mentioned tested exactly as expected though. 👍 Issues found:
We're adding a date-based filter onto the events search to prevent that (I don't know if that can be easily applied here; I haven't looked at code yet). It should be less of an issue in production since the daily import function should be unpublishing/moving expired events.
Subjective thoughts:
|
Huh yeah, that's strange. I'll look into that.
Thanks, I'll look into that.
I'll keep it on the filter.
Thanks, I'll see what I can do.
Good catch
That would be a good belt and suspenders thing to add.
I caught that too. Added some more validation in the middleware: https://github.com/SU-SWS/adapt-algolia-index/pull/120/commits/6fb73747a0e913f32ae4a14fbecfe22740eb1f3c
The scroll kinda bugs me overall. Perhaps I just remove it.
Yeah, maybe I'll two step this. Opacity immediately, and skeles for hung.
Yuuuuuge |
@@ -120,7 +120,7 @@ const EventDiscoveryContent = () => { | |||
<Hits | |||
hitComponent={Hit} | |||
classNames={{ | |||
root: `${isStalled ? 'su-opacity-50' : ''}`, | |||
root: `${isStalled ? 'su-opacity-30' : ''}`, |
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.
@ericbakenhus 50 didn't seem like enough on my monitor.
|
||
// Filter out any items we don't want to show. | ||
const filteredItems = items.filter((item) => !excludes.includes(item.value)); | ||
// Sort the items alphabetically. | ||
filteredItems.sort((a, b) => a.value.localeCompare(b.value)); | ||
|
||
// Show loading. | ||
if (status === 'loading') { | ||
if (isStalled) { |
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.
Set the skeles to show when stalled (>2s)
Before that, the opacity is set to 30.
@@ -56,11 +56,12 @@ const SearchPage = (props) => { | |||
<InstantSearch | |||
searchClient={algoliaClient} | |||
indexName={indexName} | |||
stalledSearchDelay={3000} | |||
stalledSearchDelay={2000} |
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.
Stall and show the skeles at 2s
@@ -20,13 +20,14 @@ const SearchField = ({ emptySearchMessage }) => { | |||
const index = algoliaClient.initIndex(indexName); | |||
|
|||
// Hooks and state. | |||
const { query, refine } = useSearchBox(); | |||
const { query, refine, clear } = useSearchBox(); |
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.
Hey, would you look at that! A clear function. Might be a good fit for the clear link? 🤦
@@ -44,14 +45,27 @@ const SearchField = ({ emptySearchMessage }) => { | |||
return; | |||
} | |||
|
|||
// Add the filters to the autocomplete as well. |
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.
I've patched in the facets so that they filter the autocomplete results and don't show anything outside of what is available. Before this, if I filtered down and then used the autocomplete to select an item outside of the current filter range, I got a no results found message.
@@ -19,19 +19,19 @@ describe('Travel-Study Destinations Page', () => { | |||
cy.wait(1000); | |||
|
|||
// Select Filter Month and check for URL update | |||
cy.get('[data-test="filter-option--october"]').first().check({ force: true }); | |||
cy.url().should('contain', 'trip-month=oct'); | |||
cy.get('[data-test="filter-option--february"]').first().check({ force: true }); |
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.
No upcoming october trips. 🤷
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.
I'm not sure if I've committed a crime here or not but this is helping the search page as the anchor links are triggering gatsby's (reach) router to do a 'navigate' and render. The change to the URL is also picked up by the history
function in the instantsearch router.
By putting the path into the URL and omitting the default browser behaviour I skip the route page render but I am still getting the history
event and the search still searches. At least with this code the search and facets still update the URL when they are interacted with.
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.
It results in all the same expected behavior, so I'd say it's legal. 😆
Co-authored-by: Eric Bakenhus <[email protected]>
@ericbakenhus and @yvonnetangsu I feel 'good enough' with this version. I've added a couple of follow-up tickets for the next sprint. Thanks for all of your review and feedback |
Ah shoot. Forgot to change the index back! |
READY FOR REVIEW
Summary
Review By (Date)
Criticality
Review Tasks
1. Test Modal Desktop
esc
to close the overlaygoose
or something elseesc
to close autocomplete optiondown arrow
to open autocomplete optionclear x
option shows up in the search field/search
with a?q=
parameter that matches your search string2. Test Modal Mobile
3. Test Search Page
reset all filters
link and assert that the filters are removedAlumni Events
/events
finderWine
and ensure that you are shown the Stanford Wine Collection purple banner4. Test Storyblok Configuration
5. Aesthetic / A11y / Code
BUGS