Skip to content

similar resources carousel #1835

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

Merged
merged 15 commits into from
Nov 21, 2024
Merged

Conversation

gumaerc
Copy link
Contributor

@gumaerc gumaerc commented Nov 20, 2024

What are the relevant tickets?

Closes https://github.com/mitodl/hq/issues/5878

Description (What does it do?)

This PR adds two ResourceCarousels to the new learning resource drawer based on the new "similar resources" APi endpoints. Both the opensearch and vector based results are displayed below the resource information.

Screenshots (if appropriate):

image
image

How can this be tested?

  • If you have Posthog set up locally, enable the lr_drawer_v2 flag
  • If you don't have Posthog set up locally, you can force drawerV2 to be true in LearningResourceDrawer.tsx
  • Spin up this branch of mit-learn
  • Generate vector embeddings by running docker compose exec web python manage.py generate_embeddings --skip-contentfiles --recreate-collections --all
  • Visit the search page at http://localhost:8062/search
  • View a variety of learning resources and ensure that the related resources carousels appear
  • Click the cards in the carousels to navigate to other learning resources and ensure that the drawer reloads with the new resource seamlessly

@gumaerc gumaerc force-pushed the cg/v2-drawer-similar-resources-carousel branch 2 times, most recently from 8927df5 to 8a1d565 Compare November 20, 2024 23:43
@gumaerc gumaerc marked this pull request as ready for review November 20, 2024 23:43
@shanbady shanbady self-requested a review November 21, 2024 14:18
`${API_BASE_URL}/api/v1/learning_resources/${params.id}/similar/`,
vectorSimilar: (
params: Params<LRApi, "learningResourcesVectorSimilarList">,
) => `${API_BASE_URL}/api/v1/learning_resources/${params.id}/vector_similar/`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I scroll all the way to the end of the carousel it seems to allow for one extra (and empty) pagination. Should we fetch some multiple of 4 (can be done via limit get parameter) so it evenly pages?
Screenshot 2024-11-21 at 11 53 57 AM

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sovsey What do you think?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's update so it evenly pages.

@@ -65,6 +66,7 @@ const DrawerContent: React.FC<{
closeDrawer: () => void
}> = ({ resourceId, closeDrawer }) => {
const resource = useLearningResourcesDetail(Number(resourceId))
// const similarResources = useSimilarLearningResources(Number(resourceId))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stray comment

@@ -80,6 +80,7 @@ const IconContainer = styled.span({
[theme.breakpoints.down("sm")]: {
display: "none",
},
zIndex: -1,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this due to an issue with some elements of the drawer ending up "on top" of the sticky header in the drawer when you scroll down. I changed the approach here and instead set zIndex: 1 on the header itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's an example I captured on RC of the icons from InfoSection flowing over onto the header:
chrome_HBOmZykrsN

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize this seems somewhat unrelated, but now that the carousels will be in there scrolling will actually happen in the drawer and these issues made themselves apparent, so I'd like to get this fixed as part of adding the carousels.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good. just wanted to make sure it was something intentional and not left behind from previous testing.

@gumaerc gumaerc force-pushed the cg/v2-drawer-similar-resources-carousel branch from eb9d233 to e1d5119 Compare November 21, 2024 18:21
@gumaerc gumaerc added Needs Review An open Pull Request that is ready for review and removed Waiting on author labels Nov 21, 2024
@gumaerc
Copy link
Contributor Author

gumaerc commented Nov 21, 2024

@shanbady This is ready for another look pending whatever @sovsey thinks we should do re: the empty space at the end of the carousel

Copy link
Contributor

@shanbady shanbady left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Also worth mentioning @sovsey the fix to resolve the performance issue on the vector results is up at #1839 - if we have made a decision on the endpoint, we can use the vector based results once that pr is merged

@gumaerc
Copy link
Contributor Author

gumaerc commented Nov 21, 2024

@sovsey I'm going to merge this. If we decide we want to even out the amount of cards in the carousels as @shanbady suggested, I can put up a separate PR for that.

@gumaerc gumaerc merged commit c3ccd2f into main Nov 21, 2024
11 checks passed
mbertrand pushed a commit that referenced this pull request Nov 22, 2024
* reconfigure API endpoints to not be paginated, support similar resources query and add to v2 drawer

* style the carousel

* retrieve not list

* set anonymous read only permission on new endpoints

* put the api endpoints back the way they were

* fix js tests

* make carousels optional

* more styling work

* flex grow the carousel section

* add vector based similar learning resources carousel

* use correct type

* add tests

* adjust carousel section mobile padding

* instead of setting z index on other elements within the drawer to get them below the header, raise the header itself

* remove stray comment
@odlbot odlbot mentioned this pull request Nov 25, 2024
19 tasks
@rhysyngsun rhysyngsun deleted the cg/v2-drawer-similar-resources-carousel branch February 7, 2025 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review An open Pull Request that is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants