Skip to content
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

Merge course popovers #551

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open

Merge course popovers #551

wants to merge 19 commits into from

Conversation

CadenLee2
Copy link
Contributor

@CadenLee2 CadenLee2 commented Jan 11, 2025

Merge the course info popover and the missing prerequisites popover into one, merge their icons, refactor this into a new component, update styles

Description

Currently, there are two popovers for a course: the course information popover, and the warning popover for missing prerequisites.

  • This PR merges the two popovers into one, which displays course information and a warning for missing prerequisites if they exist
  • The two hoverable icons (info circle and warning triangle) are also merged into one, which is a warning triangle if missing prerequisites exist and a blue info circle otherwise
  • The popover explicitly opens to the left in the search results and to the right in the roadmap so as not to go off screen
    • A note about code: in any Course where we want the popover to open to the left, the prop openPopoverLeft={true} must be explicitly set, because Boostrap's auto placement was broken and it often went off screen. If openPopoverLeft is not included, the popover will open to the right.
    • On mobile, the popover always opens to the bottom, regardless of whether it was set to open on the left
  • The merged course popover is now a new component, CoursePopover
  • The popover styles/organization mostly conform to those in the Figma design (note that certain features shown in the Figma like the quarter offerings list are not yet added, and that the list of all prerequisites is currently still included because it is easy to remove later if wanted)
  • ppc-popover is now an available CSS class in App.scss for such popovers (React Bootstrap popovers)

Screenshots

Before:
Screenshot 2025-01-10 220017
Screenshot 2025-01-10 220259
After:
Screenshot 2025-01-10 215959
Dark mode, and opening on the left side (from the roadmap search) (note that the hoverable icon looks small due to the quarter offerings and small width, which will be fixed later when the Roadmap Search Redesign PR is merged):
Screenshot 2025-01-10 220514
Opening on the bottom in mobile:
Screenshot 2025-01-13 112528

Test Plan

Check the above changes in the staging instance:

  • On desktop and mobile
  • In light and dark mode
  • For normal courses and courses with missing prerequisites
  • With a small screen

Verify successful deployment

Issues

Closes #526

…to merge-course-popovers

update from main
box-shadow: 0 0 16px 0 rgba(0, 0, 0, 0.35);
max-width: 375px;
margin-left: 16px;
top: -26px !important;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on research into the Boostrap Popover, it seems this is needed, but using !important is typically not good practice

Tried using margin-top, different positions, transform: translate (these do nothing)

Tried:

div.ppc-popover {
  /* ... */
  top: -26px;
}

(this does nothing)

Tried:

.ppc-popover {
  /* ... */
  &::before {
    display: block;
    content: "";
    margin-top: -26px;
  }
}

This results in partial success, but does not render the shadow or border correctly on the top portion (especially visible in light mode)--the shadow is vertically too short, so it would take multiple overlapping and offset shadows to cover that portion

Suggestions would be appreciated

Copy link
Member

Choose a reason for hiding this comment

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

yeah it looks like it was styled on the element itself, so !important is the only way to override that

<OverlayTrigger
trigger={['hover', 'focus']}
placement={isMobile ? 'bottom' : openPopoverLeft ? 'left-start' : 'right-start'}
overlay=<Popover className="ppc-popover" id={'course-popover-' + id}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When extracting CoursePopover into its own component, I originally included the top-level Popover tag in it. However, because of the ways OverlayTrigger interacts with whatever is passed into overlay to provide the placement direction and location, it failed to pass that information. This led to the popover always being located in the top-left corner of the page.

There is little documentation on how the OverlayTrigger works with what's passed into overlay, and all the examples I found put the component in the same file (often just inline), like it was before in this file. Solutions I found online included wrapping the component in a div (overlay=<div><MyComponent /></div>), or trying to pass all of the props to the component using the spread operator, but that messed up the positioning of the Popover further.

I didn't want to overcomplicate things, so this led to putting the top-level Popover in this file, then letting the CoursePopover represent its body and actual contents. That said, this is a bit unintuitive in terms of usage, and ideally CoursePopover should encapsulate the entire Popover functionality.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah. to be honest, I think we might just want to create our own custom popover component, especially if we're going to need to be able to click on things inside the popover (bookmark button) without it closing on its own

@CadenLee2 CadenLee2 marked this pull request as ready for review January 13, 2025 19:26
Copy link
Member

@Awesome-E Awesome-E left a comment

Choose a reason for hiding this comment

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

wrapped a couple elements in braces {} and tweaked some styles. just check it, make any fixes, and feel free to merge

@CadenLee2
Copy link
Contributor Author

After updating from main (the Roadmap Search Redesign), there is now a problem with the popover positions, currently unpredictable, and it should be resolved before this is merged because it can severely impact the usage of the website.

Description

  • Sometimes (reason/exact conditions unknown), the popovers for courses in the roadmap open in the top-left corner of the page rather than their correct positions
  • Their "placement" direction is correct still (arrow is on the correct side), they just have the wrong location

Steps to replicate (potentially):

  • Open the staging instance on a desktop browser
  • In the Roadmap search, search for ICS 33; drag it into Fall of Year 1. Notice the popover for the warning icon opens correctly (to the right).
  • Search for ICS 31; drag it into the Fall of Year 1, before ICS 33. Hover the info icon. The popover opens in the top-left corner of the page, in the correct "placement" direction (arrow on the correct side), but entirely in the wrong place. The ICS 33 popover should still be correct.
  • Now try exchanging their positions (dragging the 33 up to above the 31). Both of their popovers should be in the wrong place now.
  • After various exchanges, adding of courses, and so on, certain course popovers will, seemingly arbitrarily and inconsistently, open in the wrong place.

Notes

  • This problem only started happening after updating the from main.
  • An inconsistency potentially related to this issue seems to exist on the current production deployment, though it's not noticeable at all because the popovers keep a relatively expected position:
    • Search for any course in the roadmap in production on desktop
    • Hover over its info icon. Notice that the info popover's arrow ends exactly in the middle of the icon. Now move the mouse then re-hover over the same icon. The popover has moved slightly to the left.
    • To my knowledge, this did not exist before
  • Using developer tools to inspect, the mispositioned popovers in this branch all seem to have transform: translate(7px) set (as opposed to the correct value of the transform)
  • This affects mobile as well
  • This does not affect courses in the search results

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Merge the "Course Info" and "Prerequisite Not Met" Popovers
3 participants