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.

Sign up for GitHub

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

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
129bbbe
feat: merge warning into course popover
Cadecraft Dec 29, 2024
82498db
feat: replace info icon with warning icon if issue
CadenLee2 Dec 29, 2024
a2dd379
feat: make popover styles conform more to figma
CadenLee2 Jan 9, 2025
9cbaa93
feat: increase popover arrow size
CadenLee2 Jan 10, 2025
a5b451e
fix: correct popover directions with arrows
CadenLee2 Jan 10, 2025
e194666
fix: popover sizing details
CadenLee2 Jan 10, 2025
ae136a1
Merge branch 'main' of https://github.com/icssc/peterportal-client in…
CadenLee2 Jan 10, 2025
101a0ce
feat: popover arrow shadow
CadenLee2 Jan 10, 2025
07aeee9
refactor: move course popover to new component
CadenLee2 Jan 11, 2025
c323679
fix: course popover line heights
CadenLee2 Jan 11, 2025
de9fa61
fix: course popover props interface
CadenLee2 Jan 11, 2025
e0e2c05
fix: popover position on mobile
CadenLee2 Jan 11, 2025
025aa9e
Merge branch 'main' of https://github.com/icssc/peterportal-client in…
CadenLee2 Jan 11, 2025
98bb1dd
refactor: remove separate popover position classes
CadenLee2 Jan 13, 2025
77a1e70
Merge branch 'main' into merge-course-popovers
CadenLee2 Jan 13, 2025
92d84bb
Adjust styles of tooltip
Awesome-E Jan 13, 2025
4ec9a23
fix tooltips in other directions
Awesome-E Jan 13, 2025
ffb01fa
update from main
CadenLee2 Jan 16, 2025
4584700
fix: bottom popover arrow color
CadenLee2 Jan 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 57 additions & 0 deletions site/src/App.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,63 @@ button {
}
}

.ppc-popover {
border-radius: 8px;
padding: 0px;
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


.popover-body {
border-radius: 8px;
padding: 16px;
}

.arrow {
z-index: -1;
}

.arrow::after {
border-width: 20px 20px 20px;
top: 10px;
left: -32px;
pointer-events: none;
}

.arrow::before {
border-width: 20px 20px 20px;
pointer-events: none;
position: relative;
top: 10px;
left: calc(100% + 2px);
border: 14px solid var(--overlay1);
width: 28px;
height: 28px;
transform-origin: 0 0;
transform: rotate(45deg);
box-shadow: 0px 0px 16px 0px rgba(0, 0, 0, 0.35);
}
}

.bs-popover-left {
margin-right: 20px;

.arrow::after,
.arrow::before {
left: -1px;
}
}

.bs-popover-bottom {
margin-top: 48px;
.arrow::after,
.arrow::before {
top: -12px;
left: 7px;
}
}

.ppc-modal {
font-size: 18px;

Expand Down
62 changes: 62 additions & 0 deletions site/src/component/CoursePopover/CoursePopover.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
.course-popover {
line-height: normal;

.popover-name {
font-weight: bold;
color: var(--text-dark);
font-size: 16px;
height: 21px;
margin-top: -2px;
}

.popover-subtitle {
font-weight: bold;
}

.popover-units {
font-weight: normal;
}

.popover-description {
font-size: 14px;
padding-top: 12px;
line-height: 17px;
}

.popover-detail {
padding-top: 12px;
}

.popover-detail-prefix {
font-weight: bold;
}

.popover-detail-warning {
font-size: 14px;
color: #ce0000;
gap: 5px;

.popover-detail-warning-icon {
margin-bottom: 4px;
margin-right: 4px;
font-size: 16px;
}
}

.popover-detail-italics {
padding-top: 6px;
font-weight: 400;
font-size: 12px;
font-style: italic;
color: var(--petr-gray);
}
}

[data-theme='dark'] {
.popover-detail-warning {
color: red;
}
.popover-detail-italics {
color: var(--petr-gray);
}
}
68 changes: 68 additions & 0 deletions site/src/component/CoursePopover/CoursePopover.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
import { FC } from 'react';
import './CoursePopover.scss';
import { ExclamationTriangle } from 'react-bootstrap-icons';
import Popover from 'react-bootstrap/Popover';

interface CoursePopoverProps {
department: string;
courseNumber: string;
title: string;
minUnits: number;
maxUnits: number;
description: string;
prerequisiteText: string;
corequisites: string;
requiredCourses?: string[];
}

const CoursePopover: FC<CoursePopoverProps> = (props) => {
const {
department,
courseNumber,
title,
minUnits,
maxUnits,
description,
prerequisiteText,
corequisites,
requiredCourses,
} = props;

return (
<Popover.Content className="course-popover">
<div className="popover-name">
{department + ' ' + courseNumber + ' '}
<span className="popover-units">({minUnits === maxUnits ? minUnits : `${minUnits}-${maxUnits}`} units)</span>
</div>
<div className="popover-description">
<span className="popover-subtitle">{title + ':'}</span>
<br />
{description}
</div>
{prerequisiteText && (
<div className="popover-detail">
<span className="popover-detail-prefix">Prerequisites:</span> {prerequisiteText}
</div>
)}
{corequisites && (
<div className="popover-detail">
<span className="popover-detail-prefix">Corequisites:</span> {corequisites}
</div>
)}
{requiredCourses && (
<div className="popover-detail">
<div className="popover-detail-warning">
<ExclamationTriangle className="popover-detail-warning-icon" />
Prerequisite{requiredCourses?.length > 1 ? 's' : ''} Not Met: {requiredCourses?.join(', ')}
</div>
<div className="popover-detail-italics">
Already completed? Click "Transfer Credits" at the top of the roadmap viewer to add{' '}
{requiredCourses?.length > 1 ? 'these prerequisites' : 'this prerequisite'}.
</div>
</div>
)}
</Popover.Content>
);
};

export default CoursePopover;
29 changes: 0 additions & 29 deletions site/src/pages/RoadmapPage/Course.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,35 +59,6 @@
color: red;
}

.course-popover {
padding: 5%;

.popover-name {
font-weight: bold;
color: var(--text-dark);
font-size: 16px;
}

.popover-units {
color: #c4c4c4;

.popover-units-value {
font-weight: bold;
}
}

.popover-description {
padding-top: 5%;
}

.popover-detail {
padding-top: 5%;
}
.popover-detail-prefix {
font-weight: bold;
}
}

.course-footer {
height: auto;
display: flex;
Expand Down
68 changes: 24 additions & 44 deletions site/src/pages/RoadmapPage/Course.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@ import './Course.scss';
import { Button } from 'react-bootstrap';
import { InfoCircle, ExclamationTriangle, Trash, BagPlus, BagFill } from 'react-bootstrap-icons';
import CourseQuarterIndicator from '../../component/QuarterTooltip/CourseQuarterIndicator';
import CoursePopover from '../../component/CoursePopover/CoursePopover';
import OverlayTrigger from 'react-bootstrap/OverlayTrigger';
import Popover from 'react-bootstrap/Popover';
import { useIsMobile } from '../../helpers/util';

import { CourseGQLData } from '../../types/types';
import ThemeContext from '../../style/theme-context';
Expand All @@ -16,6 +18,7 @@ interface CourseProps extends CourseGQLData {
onAddToBag?: () => void;
isInBag?: boolean;
removeFromBag?: () => void;
openPopoverLeft?: boolean;
}

const Course: FC<CourseProps> = (props) => {
Expand All @@ -35,46 +38,11 @@ const Course: FC<CourseProps> = (props) => {
onAddToBag,
isInBag,
removeFromBag,
openPopoverLeft,
} = props;
const CoursePopover = (
<Popover id={'course-popover-' + id}>
<Popover.Content>
<div className="course-popover">
<div className="popover-name">
{department + ' ' + courseNumber} {title}
</div>
<div className="popover-units">
<span className="popover-units-value">{minUnits === maxUnits ? minUnits : `${minUnits}-${maxUnits}`}</span>{' '}
units
</div>
<div className="popover-description">{description}</div>
{prerequisiteText && (
<div className="popover-detail">
<span className="popover-detail-prefix">Prerequisites:</span> {prerequisiteText}
</div>
)}
{corequisites && (
<div className="popover-detail">
<span className="popover-detail-prefix">Corequisites:</span> {corequisites}
</div>
)}
</div>
</Popover.Content>
</Popover>
);

const WarningPopover = (
<Popover id={'warning-popover-' + id}>
<Popover.Content>
Prerequisite(s) not met! Missing: {requiredCourses?.join(', ')}
<br />
Already completed prerequisite(s) at another institution? Click 'Transfer Credits' at the top of the planner to
clear the prerequisite(s).
</Popover.Content>
</Popover>
);

const courseRoute = '/course/' + props.department.replace(/\s+/g, '') + props.courseNumber.replace(/\s+/g, '');
const isMobile = useIsMobile();

return (
<div className={`course ${requiredCourses ? 'invalid' : ''}`}>
Expand All @@ -86,14 +54,26 @@ const Course: FC<CourseProps> = (props) => {
</a>
<span className="units">, {minUnits === maxUnits ? minUnits : `${minUnits}-${maxUnits}`} units</span>
</span>
<OverlayTrigger trigger={['hover', 'focus']} placement="auto" overlay={CoursePopover} delay={100}>
<InfoCircle />
<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

<CoursePopover
department={department}
courseNumber={courseNumber}
title={title}
minUnits={minUnits}
maxUnits={maxUnits}
description={description}
prerequisiteText={prerequisiteText}
corequisites={corequisites}
requiredCourses={requiredCourses}
/>
</Popover>
delay={100}
>
{requiredCourses ? <ExclamationTriangle /> : <InfoCircle />}
</OverlayTrigger>
{requiredCourses && (
<OverlayTrigger trigger={['hover', 'focus']} placement="right" overlay={WarningPopover} delay={100}>
<ExclamationTriangle />
</OverlayTrigger>
)}
</div>
{onDelete ? (
<ThemeContext.Consumer>
Expand Down
1 change: 1 addition & 0 deletions site/src/pages/RoadmapPage/CourseBag.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ const CourseBag = () => {
onDelete={() => {
removeCourseFromBag(course);
}}
openPopoverLeft={true}
/>
</div>
)}
Expand Down
8 changes: 7 additions & 1 deletion site/src/pages/RoadmapPage/CourseHitItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,13 @@ const CourseHitItem: FC<CourseHitItemProps> = (props: CourseHitItemProps) => {
...provided.draggableProps.style,
}}
>
<Course {...props} onAddToBag={onAddToBag} isInBag={isInBag} removeFromBag={removeFromBag} />
<Course
{...props}
onAddToBag={onAddToBag}
isInBag={isInBag}
removeFromBag={removeFromBag}
openPopoverLeft={true}
/>
</div>
);
}}
Expand Down
3 changes: 3 additions & 0 deletions site/src/style/theme.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,9 @@
}
}

.popover {
background-color: var(--overlay1);
}
.popover-body {
background-color: var(--overlay1);
color: var(--text);
Expand Down
Loading