-
Notifications
You must be signed in to change notification settings - Fork 81
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
feat: improve search modal results [FC-0040] #946
Changes from 2 commits
14ded36
cd66611
f4cf7fd
53d70e3
6cb2d96
f1b3b62
c152a42
afa8fdd
8e468ef
079e52e
78e0a81
8c75c8d
68da2b9
879a395
66612bc
58a5c8c
7d037f2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
declare module '*.svg' { | ||
const content: string; | ||
export default content; | ||
} | ||
Comment on lines
+1
to
+4
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To support importing |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,30 @@ | ||
/* eslint-disable react/prop-types */ | ||
// @ts-check | ||
import React from 'react'; | ||
import { FormattedMessage } from '@edx/frontend-platform/i18n'; | ||
import { Stack } from '@openedx/paragon'; | ||
import { useStats, useClearRefinements } from 'react-instantsearch'; | ||
|
||
import EmptySearchImage from './images/empty-search.svg'; | ||
import NoResultImage from './images/no-results.svg'; | ||
import messages from './messages'; | ||
|
||
const EmptySearch = () => ( | ||
<Stack className="d-flex mt-6 align-items-center"> | ||
<p className="fs-large"> <FormattedMessage {...messages.emptySearchTitle} /> </p> | ||
<p className="small text-muted"> <FormattedMessage {...messages.emptySearchSubtitle} /> </p> | ||
<img src={EmptySearchImage} alt="" /> | ||
</Stack> | ||
); | ||
|
||
const NoResults = () => ( | ||
<Stack className="d-flex mt-6 align-items-center"> | ||
<p className="fs-large"> <FormattedMessage {...messages.noResultsTitle} /> </p> | ||
<p className="small text-muted"> <FormattedMessage {...messages.noResultsSubtitle} /> </p> | ||
<img src={NoResultImage} alt="" /> | ||
</Stack> | ||
); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think these should be deduplicated since they have the same structure, and will likely need to always have the same structure. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done: 78e0a81 |
||
/** | ||
* If the user hasn't put any keywords/filters yet, display an "empty state". | ||
* Likewise, if the results are empty (0 results), display a special message. | ||
|
@@ -16,12 +38,10 @@ const EmptyStates = ({ children }) => { | |
|
||
if (!hasQuery && !hasFiltersApplied) { | ||
// We haven't started the search yet. Display the "start your search" empty state | ||
// Note this isn't localized because it's going to be replaced in a fast-follow PR. | ||
return <p className="text-muted text-center mt-6">Enter a keyword or select a filter to begin searching.</p>; | ||
return <EmptySearch />; | ||
} | ||
if (nbHits === 0) { | ||
// Note this isn't localized because it's going to be replaced in a fast-follow PR. | ||
return <p className="text-muted text-center mt-6">No results found. Change your search and try again.</p>; | ||
return <NoResults />; | ||
} | ||
|
||
return children; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -64,8 +64,7 @@ | |
} | ||
} | ||
|
||
.ais-InfiniteHits-loadPrevious, | ||
.ais-InfiniteHits-loadMore--disabled { | ||
display: none; // temporary; remove this once we implement our own <Hits>/<InfiniteHits> component. | ||
.fs-large { | ||
font-size: $font-size-base * 1.25; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can use the class There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice! Thank you! |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,6 +23,11 @@ const queryClient = new QueryClient({ | |
}, | ||
}); | ||
|
||
jest.mock('react-redux', () => ({ | ||
...jest.requireActual('react-redux'), | ||
useSelector: jest.fn(), | ||
})); | ||
|
||
const RootWrapper = () => ( | ||
<AppProvider store={store}> | ||
<IntlProvider locale="en" messages={{}}> | ||
|
@@ -59,7 +64,7 @@ describe('<SearchModal />', () => { | |
apiKey: 'test-api-key', | ||
}); | ||
const { findByText } = render(<RootWrapper />); | ||
expect(await findByText('Enter a keyword or select a filter to begin searching.')).toBeInTheDocument(); | ||
expect(await findByText('Start searching to find content')).toBeInTheDocument(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add more tests to cover the added functionality for this SearchModal? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
}); | ||
|
||
it('should render the spinner while the config is loading', () => { | ||
|
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 the animation after scrolling to a searched component. Personally, I didn't like it. 😞
I am open to suggestions! @ali-hugo
PS: The highlight only happens if the parent item is expanded and the component is on screen. We could improve it with a follow-up task.
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.
@rpenido I agree that the scrolling is awkward.. but I can't think of a better place to send someone who's searched for a Section/Subsection.
However, if they search for a Unit, I think we should direct them to the Unit edit page instead of scrolling down the outline, because Units are very unlikely to be expanded on that page. So then it's just Sections and Subsections that use the scrolling thing, and Units and components redirect to the Unit edit page.
@ali-hugo What do you think?
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.
The Unit redirect will be fixed in a follow-up task!
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.
What seems to be the issue with the animation? I think one thing that can be done to make it smoother is to use a timing function.
i.e. remove the 90% step and add a timing function like:
animation-timing-function: cubic-bezier(1,0,.72,.04);
This will be a lot smoother I think since it'll follow a curve rather than a steep fall.
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.
Nothing specific. I just didn't find it "wow" (I'm not sure if it was the color, the glow size..). I think it's a subjective feeling, and I'm open to options to improve it (if that's the case).
Changed the curve here: 12ba0c5