Skip to content

Conversation

@gumaerc
Copy link
Contributor

@gumaerc gumaerc commented Oct 18, 2024

What are the relevant tickets?

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

Description (What does it do?)

This PR is a first pass at implementing the new wider learning resource drawer design. The structure and styles have been altered to match what's included in the design, minus the out of scope items listed in the issue. As part of this, the dropdown UI for selecting runs that changes the information section has been removed. Instead, all possible values across all runs are shown in a pipe separated list.

Screenshots (if appropriate):

image
image

How can this be tested?

  • Spin up mit-learn on this branch
  • Ensure that your local database is sufficiently populated with enough learning resources for testing:
    • A variety of courses
    • Podcasts
    • Videos
  • Ensure that you have built a search index
  • Visit the search page at http://localhost:8062/search
  • Search for and click on learning resources to bring up the new drawer
  • Check all of the above resource type and ensure that the general structure and information presented matches what's expected and aligns with the provided designs

@gumaerc gumaerc force-pushed the cg/new-learning-resource-drawer-layout branch from 866d372 to a16fc3b Compare October 21, 2024 16:58
@gumaerc gumaerc marked this pull request as ready for review October 21, 2024 20:10
@gumaerc gumaerc force-pushed the cg/new-learning-resource-drawer-layout branch from f38a1e3 to 9f6e1d4 Compare October 22, 2024 14:36
@gumaerc gumaerc added Needs Review An open Pull Request that is ready for review and removed Work in Progress labels Oct 22, 2024
@gumaerc gumaerc force-pushed the cg/new-learning-resource-drawer-layout branch from 856b183 to fe3a816 Compare October 22, 2024 20:11
@abeglova abeglova self-assigned this Oct 23, 2024
Copy link
Contributor

@abeglova abeglova left a comment

Choose a reason for hiding this comment

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

Overall looks good. Did catch a couple of bugs

In mobile mode, the drawer is too wide for some programs and video. It looks ok for other types of resources

I'm not sure why this video is cut off
Screenshot 2024-10-23 at 3 18 47 PM

But this one is fine
Screenshot 2024-10-23 at 3 25 47 PM

<InfoSection resource={resource} />
</LeftContainer>
<RightContainer>
<CallToActionSection
Copy link
Contributor

Choose a reason for hiding this comment

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

This adds a call to action for all resource types. The button does not go anywhere for video playlists and learning paths since url is null for those resources and getCallToActionUrl only has special logic for podcasts. http://api.open.odl.local:8063/api/v1/learning_resources_search/?resource_type=video_playlist . You can add special logic to construct the correct url based on the channel_id for the video playlist. I'm not sure if we are supposed to have learning paths in prod. There is currently one in the production search but I think that it might not be supposed to be visible

@abeglova abeglova added Waiting on author and removed Needs Review An open Pull Request that is ready for review labels Oct 23, 2024
@gumaerc
Copy link
Contributor Author

gumaerc commented Oct 23, 2024

Note to self: fix alignment of bookmark buttons when "on platform" section is not present.

@gumaerc gumaerc force-pushed the cg/new-learning-resource-drawer-layout branch 5 times, most recently from 61e8703 to 542a661 Compare October 25, 2024 14:36
@gumaerc gumaerc added Needs Review An open Pull Request that is ready for review and removed Waiting on author labels Oct 25, 2024
@gumaerc
Copy link
Contributor Author

gumaerc commented Oct 25, 2024

@abeglova Thanks for your review, this is ready for another look. The mobile video issue actually had to do with the description on that particular video causing the description field to overflow the flex parent because of the white-space: pre-line rule. I changed this to use white-space: pre-wrap and word-break: break-word to fix this. Really the issue was caused by a really long unbroken line of text that was a URL. In reality, URLs like this should be actual links but I don't think we're set up for that yet. Video playlists should now have a proper CTA as well as thumbnail image. Learning paths we will leave as-is, as we discussed.

Copy link
Contributor

@abeglova abeglova left a comment

Choose a reason for hiding this comment

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

lgtm

Comment on lines 114 to 117
<ActionButton
style={closeSx}
variant="text"
size="medium"
onClick={setOpen.off}
aria-label="Close"
>
<CloseIcon />
</ActionButton>
{!hideCloseButton && (
<CloseButton
variant="text"
size="medium"
onClick={setOpen.off}
aria-label="Close"
>
<CloseIcon />
</CloseButton>
)}
Copy link
Contributor

Choose a reason for hiding this comment

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

The only instance we have of RoutedDrawer sets hideCloseButton={true}... Can we skip changing the close button style here?

@gumaerc gumaerc force-pushed the cg/new-learning-resource-drawer-layout branch 3 times, most recently from f0036c9 to 9c23f91 Compare October 30, 2024 15:06
@ChristopherChudzicki
Copy link
Contributor

@gumaerc I checked out the posthog stuff. Worked well.

@gumaerc gumaerc force-pushed the cg/new-learning-resource-drawer-layout branch from 3fb43d2 to c42b84a Compare October 31, 2024 16:38
@gumaerc gumaerc merged commit 71bee99 into main Oct 31, 2024
11 checks passed
@rhysyngsun rhysyngsun deleted the cg/new-learning-resource-drawer-layout branch February 7, 2025 20:39
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.

4 participants