-
Notifications
You must be signed in to change notification settings - Fork 33
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: display search result cards in catalog tab #1059
Conversation
src/components/learner-credit-management/search/CatalogSearchResults.jsx
Outdated
Show resolved
Hide resolved
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #1059 +/- ##
==========================================
+ Coverage 83.72% 83.78% +0.05%
==========================================
Files 449 450 +1
Lines 9531 9550 +19
Branches 1988 1994 +6
==========================================
+ Hits 7980 8001 +21
+ Misses 1508 1506 -2
Partials 43 43
☔ View full report in Codecov by Sentry. |
src/components/learner-credit-management/search/CatalogSearch.jsx
Outdated
Show resolved
Hide resolved
|
||
import { FormattedMessage } from '@edx/frontend-platform/i18n'; | ||
import { SearchHeader } from '@edx/frontend-enterprise-catalog-search'; | ||
|
||
import { configuration } from '../../../config'; | ||
import CatalogSearchResults from './CatalogSearchResults'; | ||
|
||
const CatalogSearch = () => { | ||
const { budgetId } = useParams(); | ||
const CatalogSearch = ({ catalogUuid }) => { |
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.
[question] Is it expected that mousing out of the window and back into this component would trigger another more queries to Algolia? As seen below with the "Network" tab open, it looks like there are Algolia queries
API calls being made when the user's cursor leaves the window and then again when subsequently re-entering. It's especially noticeable when the "Network" tab is throttled to "Fast 3G" or "Slow 3G", as the skeleton loading states become visible.
algolia_queries_catalog_lcm.mov
algolia_queries_catalog_lcm_2.mov
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 think this is happening because the page is re-rendering every time the user navigates to the sidebar and back. It looks like this is a larger issue with the app because I'm seeing other pages re-render 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.
Screen.Recording.2023-10-26.at.2.55.37.PM.mov
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.
Ugh, yeah, you're correct. Didn't notice it was impacting other pages at the time I left the comment; we can defer on this issue within the PR.
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.
LGTM! Just a couple nit-level comments and a clarifying question about this thread (will follow-up in Slack).
|
||
import { FormattedMessage } from '@edx/frontend-platform/i18n'; | ||
import { SearchHeader } from '@edx/frontend-enterprise-catalog-search'; | ||
|
||
import { configuration } from '../../../config'; | ||
import CatalogSearchResults from './CatalogSearchResults'; | ||
|
||
const CatalogSearch = () => { | ||
const { budgetId } = useParams(); | ||
const CatalogSearch = ({ catalogUuid }) => { |
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.
Ugh, yeah, you're correct. Didn't notice it was impacting other pages at the time I left the comment; we can defer on this issue within the PR.
import defaultLogoImg from '../../../static/default-card-header-dark.png'; | ||
import defaultCardImg from '../../../static/default-card-header-light.png'; |
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 don't feel we do, but could convinced otherwise. The fallbackSrc
is important because without it, it messes up the card grid with one or more cards with differing heights without the image. Having a missing logo image doesn't affect the card heights in the same way, so having a fallback isn't as important.
const searchClient = algoliasearch(configuration.ALGOLIA.APP_ID, configuration.ALGOLIA.SEARCH_API_KEY); | ||
|
||
const searchFilters = `enterprise_catalog_query_uuids:${budgetId}`; | ||
const searchFilters = `enterprise_catalog_uuids:${catalogUuid}`; |
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.
Yes, we want the search results to include all courses, including both course types (i.e., Open Courses and Executive Education). From a data perspective, these are both still courses, though they have different learning types. We want to exclude programs and pathways.
const searchFilters = `enterprise_catalog_uuids:${catalogUuid} AND content_type: course`;
Description
Display formatted search results on budget detail "Catalog" tab for top-down assignment based off the search filtering/search results from ticket https://2u-internal.atlassian.net/browse/ENT-7339.
Solution
Implement Paragon's Datatable and cards with the appropriate badges for exec ed and open courses.
Widescreen
Narrow screen
Datatable
For all changes
Only if submitting a visual change