Skip to content

Conversation

@ChristopherChudzicki
Copy link
Contributor

@ChristopherChudzicki ChristopherChudzicki commented Sep 18, 2024

What are the relevant tickets?

Followup to #1565

Description (What does it do?)

This PR has no functionality change, but makes some rebasing / conflicts easier on the NextJS branch. This PR tweaks two changes in introduced in #1565. Two changes introduced there were:

  1. moving SignupPopover to ol-components
    • this had the undesirable consequence that SignupPopover no longer owns the signup url, which seems appropriate. Additionally, many more components are accessing useLocation, which caused some conflicts on the nextjs branch.
  2. introducing a MUI_DRAWER_Z_INDEX constant.
    • this is more simply accessed via theme.zIndex.drawer

Change (1) was made so that LearningResourceExpanded could use the signup popover . This PR moves that logic to the LearningResourceDrawer`, which is more similar to how we handle the signup popover for learning resource cards and is also how we handle the "add to list dialogs".

How can this be tested?

  1. As an unauthenticated user, check that the signup popover opens when...
    • user clicks bookmark button on cards OR in resource drawer
    • user clicks subscription button on channel pages
  2. Check that the add to list dialogs still open appropriately.

@ChristopherChudzicki ChristopherChudzicki marked this pull request as ready for review September 18, 2024 15:14
@ChristopherChudzicki ChristopherChudzicki added the Needs Review An open Pull Request that is ready for review label Sep 18, 2024
@ChristopherChudzicki ChristopherChudzicki changed the title Cc/signup popover owns url Signup popover owns url Sep 18, 2024
@abeglova abeglova self-assigned this Sep 19, 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.

👍

@ChristopherChudzicki ChristopherChudzicki merged commit 73d430a into main Sep 19, 2024
@odlbot odlbot mentioned this pull request Sep 19, 2024
5 tasks
@rhysyngsun rhysyngsun deleted the cc/signup-popover-owns-url branch February 7, 2025 20:36
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