Skip to content

show more button for v2 drawer dates #1809

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 19 commits into from
Nov 13, 2024
Merged

show more button for v2 drawer dates #1809

merged 19 commits into from
Nov 13, 2024

Conversation

gumaerc
Copy link
Contributor

@gumaerc gumaerc commented Nov 8, 2024

What are the relevant tickets?

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

Description (What does it do?)

This PR adds a "show more" link to the start dates section of the info displayed in the v2 learning resource drawer. This expands and collapses the start dates for the course.

Screenshots (if appropriate):

image
image
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
  • Ensure you have sufficient data backpopulated into your local instance, especially Sloan courses
  • Search for a course with identical data but with multiple start dates, like "Artificial Intelligence in Pharma and Biotech" for example
  • Confirm that you can click "Show more" to expand the rest of the start dates, that the link text changes to "Show less" after it expands, and that you can click "Show less" to collapse back down
  • Search for a course with different data and multiple start dates, such as "Leading the AI-Driven Organization"
  • Confirm that all of the start dates are shown without the show more button

@gumaerc gumaerc added the Needs Review An open Pull Request that is ready for review label Nov 8, 2024
Copy link
Contributor

@jonkafton jonkafton left a comment

Choose a reason for hiding this comment

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

A couple of questions that may or may not lead to changes - looks good.

@jonkafton jonkafton added Waiting on author and removed Needs Review An open Pull Request that is ready for review labels Nov 12, 2024
@gumaerc gumaerc force-pushed the cg/lrd-v2-show-more branch from 04c3874 to b663c2f Compare November 12, 2024 17:11
@gumaerc gumaerc added Needs Review An open Pull Request that is ready for review and removed Waiting on author labels Nov 12, 2024
@gumaerc
Copy link
Contributor Author

gumaerc commented Nov 12, 2024

@jonkafton This is ready for another look. I also noticed another issue with the CTA image and couldn't help myself but to fix it. Remember how you discovered that issue where it was floating above the sticky header when scrolling, and I put in a fix to move it below with z-index: -1? Well, that's still necessary, but I noticed that although the issue had been fixed visually I was unable to click the X to close the drawer when the image was sitting under it, because the NextImage is position: absolute by default. The existing code was scaling the image using the aspect property by calculating a padding offset. Since aspect-ratio is usable in all browsers, I fixed it by applying this style to NextImage:

const Image = styled(NextImage)<{ aspect: number }>`
  position: relative !important;
  border-radius: 8px;
  width: 100%;
  aspect-ratio: ${({ aspect }) => aspect};
  object-fit: cover;
  z-index: -1;
`

@jonkafton
Copy link
Contributor

Since aspect-ratio is usable in all browsers, I fixed it by applying this style to NextImage

Better, plus we use elsewhere. The % padding does feel like a trick.

@gumaerc gumaerc merged commit 41d284d into main Nov 13, 2024
11 checks passed
This was referenced Nov 14, 2024
mbertrand pushed a commit that referenced this pull request Nov 22, 2024
* break out learning resource run comparison into a utility function

* add show more functionality for start dates

* remove separator at the end before show more / show less button

* don't wrap dates

* don't wrap show more button

* fix test

* fix cta image, use aspect-ratio and relative position

* put aspect on img element instead of containing div so it works in chrome and safari

* fix tests

* remove start date comparison as it's not supposed to work that way

* don't show dates info item at all if there are differning runs

* show less link should be on its own line

* remove unnecessary line break

* optimize the code a bit

* better name

* fix column widths in differing runs table

* refactor logic surrounding run dates

* filter out null start dates

* adjust padding above "Show less"
@rhysyngsun rhysyngsun deleted the cg/lrd-v2-show-more 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.

2 participants