From 319e6df647f2d25847c5e0e3d4314aafbd0207e9 Mon Sep 17 00:00:00 2001 From: Chris Chudzicki Date: Fri, 25 Oct 2024 16:23:47 -0400 Subject: [PATCH 01/10] make only title the anchor --- .../app-pages/HomePage/NewsEventsSection.tsx | 21 ++++-- .../src/components/Card/Card.test.tsx | 2 + .../src/components/Card/Card.tsx | 74 +++++++++++-------- 3 files changed, 61 insertions(+), 36 deletions(-) diff --git a/frontends/main/src/app-pages/HomePage/NewsEventsSection.tsx b/frontends/main/src/app-pages/HomePage/NewsEventsSection.tsx index 9c6923bd8a..47dac3ec8b 100644 --- a/frontends/main/src/app-pages/HomePage/NewsEventsSection.tsx +++ b/frontends/main/src/app-pages/HomePage/NewsEventsSection.tsx @@ -15,6 +15,7 @@ import { import type { NewsFeedItem, EventFeedItem } from "api/v0" import { formatDate } from "ol-utilities" import { RiArrowRightSLine } from "@remixicon/react" +import Link from "next/link" const Section = styled.section` background: ${theme.custom.colors.white}; @@ -111,10 +112,7 @@ const EventCard = styled(Card)` align-self: stretch; justify-content: space-between; overflow: visible; - - > a { - padding: 16px; - } + padding: 16px; ` const EventDate = styled.div` @@ -225,7 +223,16 @@ const NewsEventsSection: React.FC = () => { const EventCards = events!.results?.map((item) => ( - + { + const anchor = e.currentTarget.querySelector( + `a[href="${item.url}"]`, + ) + anchor?.click() + }} + > @@ -241,7 +248,9 @@ const NewsEventsSection: React.FC = () => { )} - {item.title} + + {item.title} + diff --git a/frontends/ol-components/src/components/Card/Card.test.tsx b/frontends/ol-components/src/components/Card/Card.test.tsx index a087bedb6d..d8e7e2cac3 100644 --- a/frontends/ol-components/src/components/Card/Card.test.tsx +++ b/frontends/ol-components/src/components/Card/Card.test.tsx @@ -3,6 +3,7 @@ import { Card } from "./Card" import React from "react" import { getOriginalSrc } from "ol-test-utilities" import invariant from "tiny-invariant" +import { ThemeProvider } from "../ThemeProvider/ThemeProvider" describe("Card", () => { test("has class MitCard-root on root element", () => { @@ -14,6 +15,7 @@ describe("Card", () => { Footer Actions , + { wrapper: ThemeProvider }, ) const card = container.firstChild as HTMLElement const title = card.querySelector(".MitCard-title") diff --git a/frontends/ol-components/src/components/Card/Card.tsx b/frontends/ol-components/src/components/Card/Card.tsx index a04dc0302a..e6c6ec0f3d 100644 --- a/frontends/ol-components/src/components/Card/Card.tsx +++ b/frontends/ol-components/src/components/Card/Card.tsx @@ -39,26 +39,24 @@ export const containerStyles = ` overflow: hidden; ` -const LinkContainer = styled(Link)` - ${containerStyles} - display: block; - position: relative; - - :hover { - text-decoration: none; - border-color: ${theme.custom.colors.silverGrayLight}; - box-shadow: - 0 2px 4px 0 rgb(37 38 43 / 10%), - 0 2px 4px 0 rgb(37 38 43 / 10%); - cursor: pointer; - } -` - -const Container = styled.div` - ${containerStyles} - display: block; - position: relative; -` +const Container = styled.div(({ theme, onClick }) => [ + { + borderRadius: "8px", + border: `1px solid ${theme.custom.colors.lightGray2}`, + background: theme.custom.colors.white, + overflow: "hidden", + display: "block", + position: "relative", + }, + onClick && { + "&:hover": { + borderColor: theme.custom.colors.silverGrayLight, + boxShadow: + "0 2px 4px 0 rgb(37 38 43 / 10%), 0 2px 4px 0 rgb(37 38 43 / 10%)", + cursor: "pointer", + }, + }, +]) const Content = () => <> @@ -140,6 +138,7 @@ type CardProps = { className?: string size?: Size href?: string + onClick?: React.MouseEventHandler } export type ImageProps = NextImageProps & { @@ -151,6 +150,7 @@ type TitleProps = { children?: ReactNode lines?: number style?: CSSProperties + "aria-label"?: string } type SlotProps = { children?: ReactNode; style?: CSSProperties } @@ -163,7 +163,7 @@ type Card = FC & { Actions: FC } -const Card: Card = ({ children, className, size, href }) => { +const Card: Card = ({ children, className, size, href, onClick }) => { let content, image: ImageProps | null = null, info: SlotProps = {}, @@ -171,8 +171,6 @@ const Card: Card = ({ children, className, size, href }) => { footer: SlotProps = {}, actions: SlotProps = {} - const _Container = href ? LinkContainer : Container - /* * Allows rendering child elements to specific "slots": * @@ -196,21 +194,33 @@ const Card: Card = ({ children, className, size, href }) => { else if (child.type === Actions) actions = child.props }) + const linkRef = React.useRef(null) + const handleLinkClick: React.MouseEventHandler = React.useCallback(() => { + linkRef.current?.click() + }, []) const allClassNames = ["MitCard-root", className ?? ""].join(" ") if (content) { return ( - <_Container className={className} href={href!}> + {content} - + ) } + const titleNode = ( + + {title.children} + + ) + + const hasHref = typeof href === "string" + return ( - <_Container href={href!} scroll={!href?.startsWith("?")}> + {image && ( // alt text will be checked on Card.Image // eslint-disable-next-line styled-components-a11y/alt-text @@ -228,16 +238,20 @@ const Card: Card = ({ children, className, size, href }) => { {info.children} )} - - {title.children} - + {hasHref ? ( + + {titleNode} + + ) : ( + titleNode + )}
{footer.children}
- + {actions.children && ( {actions.children} From ad2d0b60c4f44a8925afdcd4182b368776ca92e8 Mon Sep 17 00:00:00 2001 From: Chris Chudzicki Date: Mon, 28 Oct 2024 16:45:20 -0400 Subject: [PATCH 02/10] only use title as link --- .../src/app-pages/HomePage/HomePage.test.tsx | 4 +- .../app-pages/HomePage/NewsEventsSection.tsx | 60 ++++---- .../UserListCard/UserListCardCondensed.tsx | 22 ++- .../src/components/Card/Card.test.tsx | 50 ++++++- .../src/components/Card/Card.tsx | 137 ++++++++++++------ .../src/components/Card/ListCard.test.tsx | 54 ++++++- .../src/components/Card/ListCard.tsx | 57 +++----- .../src/components/Card/ListCardCondensed.tsx | 34 +++-- 8 files changed, 269 insertions(+), 149 deletions(-) diff --git a/frontends/main/src/app-pages/HomePage/HomePage.test.tsx b/frontends/main/src/app-pages/HomePage/HomePage.test.tsx index 6938d2be05..79d112e8ed 100644 --- a/frontends/main/src/app-pages/HomePage/HomePage.test.tsx +++ b/frontends/main/src/app-pages/HomePage/HomePage.test.tsx @@ -57,7 +57,7 @@ const setupAPIs = () => { limit: 6, sortby: "-news_date", }), - {}, + newsEvents.newsItems({ count: 0 }), ) setMockResponse.get( urls.newsEvents.list({ @@ -65,7 +65,7 @@ const setupAPIs = () => { limit: 5, sortby: "event_date", }), - {}, + newsEvents.eventItems({ count: 0 }), ) setMockResponse.get(urls.topics.list({ is_toplevel: true }), { diff --git a/frontends/main/src/app-pages/HomePage/NewsEventsSection.tsx b/frontends/main/src/app-pages/HomePage/NewsEventsSection.tsx index 47dac3ec8b..cffb7c7e4d 100644 --- a/frontends/main/src/app-pages/HomePage/NewsEventsSection.tsx +++ b/frontends/main/src/app-pages/HomePage/NewsEventsSection.tsx @@ -219,42 +219,32 @@ const NewsEventsSection: React.FC = () => { return null } - const stories = news!.results?.slice(0, 6) || [] + const stories = news.results.slice(0, 6) - const EventCards = - events!.results?.map((item) => ( - { - const anchor = e.currentTarget.querySelector( - `a[href="${item.url}"]`, - ) - anchor?.click() - }} - > - - - - {formatDate( - (item as EventFeedItem).event_details?.event_datetime, - "D", - )} - - - {formatDate( - (item as EventFeedItem).event_details?.event_datetime, - "MMM", - )} - - - - {item.title} - - - - - )) || [] + const EventCards = events.results.map((item) => ( + + + + + {formatDate( + (item as EventFeedItem).event_details?.event_datetime, + "D", + )} + + + {formatDate( + (item as EventFeedItem).event_details?.event_datetime, + "MMM", + )} + + + + {item.title} + + + + + )) return (
diff --git a/frontends/main/src/page-components/UserListCard/UserListCardCondensed.tsx b/frontends/main/src/page-components/UserListCard/UserListCardCondensed.tsx index 1689b5bd71..976e13e696 100644 --- a/frontends/main/src/page-components/UserListCard/UserListCardCondensed.tsx +++ b/frontends/main/src/page-components/UserListCard/UserListCardCondensed.tsx @@ -3,6 +3,7 @@ import { UserList } from "api" import { pluralize } from "ol-utilities" import { RiListCheck3 } from "@remixicon/react" import { ListCardCondensed, styled, theme, Typography } from "ol-components" +import Link from "next/link" const StyledCard = styled(ListCardCondensed)({ display: "flex", @@ -37,24 +38,29 @@ const IconContainer = styled.div(({ theme }) => ({ }, })) -type UserListCardCondensedProps = { - userList: U - href?: string +type UserListCardCondensedProps = { + userList: UserList + href: string className?: string } -const UserListCardCondensed = ({ +const UserListCardCondensed = ({ userList, href, className, -}: UserListCardCondensedProps) => { +}: UserListCardCondensedProps) => { return ( - - {userList.title} - + + + {userList.title} + + {userList.item_count} {pluralize("item", userList.item_count)} diff --git a/frontends/ol-components/src/components/Card/Card.test.tsx b/frontends/ol-components/src/components/Card/Card.test.tsx index d8e7e2cac3..e29f5490f3 100644 --- a/frontends/ol-components/src/components/Card/Card.test.tsx +++ b/frontends/ol-components/src/components/Card/Card.test.tsx @@ -1,9 +1,11 @@ -import { render } from "@testing-library/react" +import { render, screen } from "@testing-library/react" +import user from "@testing-library/user-event" import { Card } from "./Card" import React from "react" import { getOriginalSrc } from "ol-test-utilities" import invariant from "tiny-invariant" import { ThemeProvider } from "../ThemeProvider/ThemeProvider" +import { faker } from "@faker-js/faker/locale/en" describe("Card", () => { test("has class MitCard-root on root element", () => { @@ -39,4 +41,50 @@ describe("Card", () => { expect(footer).toHaveTextContent("Footer") expect(actions).toHaveTextContent("Actions") }) + + test("The whole card is clickable as a link", async () => { + const href = `#${faker.lorem.word()}` + render( + + Title + + Info + Footer + Actions + , + { wrapper: ThemeProvider }, + ) + // outermost wrapper is not actually clickable + const card = document.querySelector(".MitCard-root > *") + invariant(card instanceof HTMLDivElement) // Sanity: Chceck it's not an anchor + + await user.click(card) + expect(window.location.hash).toBe(href) + }) + + test("The whole card is clickable as a link when using Content, except buttons and links", async () => { + const href = `#${faker.lorem.word()}` + const onClick = jest.fn() + render( + + +
Hello!
+ + Link + + , + { wrapper: ThemeProvider }, + ) + const button = screen.getByRole("button", { name: "Button" }) + await user.click(button) + expect(onClick).toHaveBeenCalled() + expect(window.location.hash).toBe("") + + // outermost wrapper is not actually clickable + const card = document.querySelector(".MitCard-root > *") + invariant(card instanceof HTMLDivElement) // Sanity: Chceck it's not an anchor + + await user.click(card) + expect(window.location.hash).toBe(href) + }) }) diff --git a/frontends/ol-components/src/components/Card/Card.tsx b/frontends/ol-components/src/components/Card/Card.tsx index e6c6ec0f3d..e1824c7a6a 100644 --- a/frontends/ol-components/src/components/Card/Card.tsx +++ b/frontends/ol-components/src/components/Card/Card.tsx @@ -4,6 +4,7 @@ import React, { Children, isValidElement, CSSProperties, + useCallback, } from "react" import styled from "@emotion/styled" import { theme } from "../ThemeProvider/ThemeProvider" @@ -13,6 +14,30 @@ import { default as NextImage, ImageProps as NextImageProps } from "next/image" export type Size = "small" | "medium" +type LinkableProps = { + href?: string + children?: ReactNode + className?: string +} +/** + * Render a NextJS link if href is provided, otherwise a span. + * Does not scroll if the href is a query string. + */ +export const Linkable: React.FC = ({ + href, + children, + className, +}) => { + if (href) { + return ( + + {children} + + ) + } + return {children} +} + /* *The relative positioned wrapper allows the action buttons to live adjacent to the * Link container in the DOM structure. They cannot be a descendent of it as @@ -32,31 +57,25 @@ export const Wrapper = styled.div<{ size?: Size }>` }} ` -export const containerStyles = ` - border-radius: 8px; - border: 1px solid ${theme.custom.colors.lightGray2}; - background: ${theme.custom.colors.white}; - overflow: hidden; -` - -const Container = styled.div(({ theme, onClick }) => [ - { - borderRadius: "8px", - border: `1px solid ${theme.custom.colors.lightGray2}`, - background: theme.custom.colors.white, - overflow: "hidden", - display: "block", - position: "relative", - }, - onClick && { - "&:hover": { - borderColor: theme.custom.colors.silverGrayLight, - boxShadow: - "0 2px 4px 0 rgb(37 38 43 / 10%), 0 2px 4px 0 rgb(37 38 43 / 10%)", - cursor: "pointer", +export const Container = styled.div<{ display?: CSSProperties["display"] }>( + ({ theme, onClick, display = "block" }) => [ + { + borderRadius: "8px", + border: `1px solid ${theme.custom.colors.lightGray2}`, + background: theme.custom.colors.white, + display, + position: "relative", + }, + onClick && { + "&:hover": { + borderColor: theme.custom.colors.silverGrayLight, + boxShadow: + "0 2px 4px 0 rgb(37 38 43 / 10%), 0 2px 4px 0 rgb(37 38 43 / 10%)", + cursor: "pointer", + }, }, - }, -]) + ], +) const Content = () => <> @@ -81,7 +100,7 @@ const Info = styled.div<{ size?: Size }>` margin-bottom: ${({ size }) => (size === "small" ? 4 : 8)}px; ` -const Title = styled.span<{ lines?: number; size?: Size }>` +const Title = styled(Linkable)<{ lines?: number; size?: Size }>` text-overflow: ellipsis; height: ${({ lines, size }) => { const lineHeightPx = size === "small" ? 18 : 20 @@ -133,10 +152,47 @@ const Actions = styled.div` right: 16px; ` +/** + * Click the child anchor element if the click event target is not the anchor itself. + * + * Allows making a whole region clickable as a link, even if the link is not the + * direct target of the click event. + */ +export const useClickChildHref = ( + href?: string, + onClick?: React.MouseEventHandler, +): React.MouseEventHandler => { + return useCallback( + (e) => { + onClick?.(e) + const anchor = e.currentTarget.querySelector( + `a[href="${href}"]`, + ) + const target = e.target as HTMLElement + if (!anchor || target.closest("a, button")) return + if (e.metaKey || e.ctrlKey) { + const opts: PointerEventInit = { + bubbles: false, + metaKey: e.metaKey, + ctrlKey: e.ctrlKey, + } + anchor.dispatchEvent(new PointerEvent("click", opts)) + } else { + anchor.click() + } + }, + [href, onClick], + ) +} + type CardProps = { children: ReactNode[] | ReactNode className?: string size?: Size + /** + * If provided, the card will render its title as a link. The entire card will + * be clickable, activating the link. + */ href?: string onClick?: React.MouseEventHandler } @@ -150,7 +206,6 @@ type TitleProps = { children?: ReactNode lines?: number style?: CSSProperties - "aria-label"?: string } type SlotProps = { children?: ReactNode; style?: CSSProperties } @@ -194,33 +249,25 @@ const Card: Card = ({ children, className, size, href, onClick }) => { else if (child.type === Actions) actions = child.props }) - const linkRef = React.useRef(null) - const handleLinkClick: React.MouseEventHandler = React.useCallback(() => { - linkRef.current?.click() - }, []) + const hasHref = typeof href === "string" + const handleHrefClick = useClickChildHref(href, onClick) + const handleClick = hasHref ? handleHrefClick : onClick + const allClassNames = ["MitCard-root", className ?? ""].join(" ") if (content) { return ( - + {content} ) } - const titleNode = ( - - {title.children} - - ) - - const hasHref = typeof href === "string" - return ( - + {image && ( // alt text will be checked on Card.Image // eslint-disable-next-line styled-components-a11y/alt-text @@ -238,13 +285,9 @@ const Card: Card = ({ children, className, size, href, onClick }) => { {info.children} )} - {hasHref ? ( - - {titleNode} - - ) : ( - titleNode - )} + + {title.children} +
diff --git a/frontends/ol-components/src/components/Card/ListCard.test.tsx b/frontends/ol-components/src/components/Card/ListCard.test.tsx index 3fc65edb90..98d26f345a 100644 --- a/frontends/ol-components/src/components/Card/ListCard.test.tsx +++ b/frontends/ol-components/src/components/Card/ListCard.test.tsx @@ -1,17 +1,67 @@ -import { render } from "@testing-library/react" +import { render, screen } from "@testing-library/react" +import user from "@testing-library/user-event" import { ListCard } from "./ListCard" import React from "react" +import { faker } from "@faker-js/faker/locale/en" +import invariant from "tiny-invariant" +import { ThemeProvider } from "../ThemeProvider/ThemeProvider" describe("ListCard", () => { - test("has class MitCard-root on root element", () => { + test("has class MitListCard-root on root element", () => { const { container } = render( Hello world , + { wrapper: ThemeProvider }, ) const card = container.firstChild expect(card).toHaveClass("MitListCard-root") expect(card).toHaveClass("Foo") }) + + test("The whole card is clickable as a link", async () => { + const href = `#${faker.lorem.word()}` + render( + + Title + Info + Footer + Actions + , + { wrapper: ThemeProvider }, + ) + // outermost wrapper is not actually clickable + const card = document.querySelector(".MitListCard-root > *") + invariant(card instanceof HTMLDivElement) // Sanity: Chceck it's not an anchor + + await user.click(card) + expect(window.location.hash).toBe(href) + }) + + test("The whole card is clickable as a link when using Content, except buttons and links", async () => { + const href = `#${faker.lorem.word()}` + const onClick = jest.fn() + render( + + +
Hello!
+ + Link + + , + { wrapper: ThemeProvider }, + ) + const button = screen.getByRole("button", { name: "Button" }) + await user.click(button) + expect(onClick).toHaveBeenCalled() + expect(window.location.hash).toBe("") + + // outermost wrapper is not actually clickable + const card = document.querySelector(".MitListCard-root > *") + invariant(card instanceof HTMLDivElement) // Sanity: Chceck it's not an anchor + + await user.click(card) + expect(window.location.hash).toBe(href) + }) }) diff --git a/frontends/ol-components/src/components/Card/ListCard.tsx b/frontends/ol-components/src/components/Card/ListCard.tsx index 18f0ff33f1..04ecbfdadc 100644 --- a/frontends/ol-components/src/components/Card/ListCard.tsx +++ b/frontends/ol-components/src/components/Card/ListCard.tsx @@ -1,36 +1,18 @@ import React, { FC, ReactNode, Children, isValidElement } from "react" -import Link from "next/link" import styled from "@emotion/styled" import { RiDraggable } from "@remixicon/react" import { theme } from "../ThemeProvider/ThemeProvider" -import { Wrapper, containerStyles, ImageProps } from "./Card" +import { + Wrapper, + Container, + ImageProps, + useClickChildHref, + Linkable, +} from "./Card" import { TruncateText } from "../TruncateText/TruncateText" import { ActionButton, ActionButtonProps } from "../Button/Button" import { default as NextImage } from "next/image" -export const LinkContainer = styled(Link)` - ${containerStyles} - display: flex; - - :hover { - text-decoration: none; - border-color: ${theme.custom.colors.silverGrayLight}; - box-shadow: - 0 2px 4px 0 rgb(37 38 43 / 10%), - 0 2px 4px 0 rgb(37 38 43 / 10%); - cursor: pointer; - } -` - -export const Container = styled.div` - ${containerStyles} -` - -export const DraggableContainer = styled.div` - ${containerStyles} - display: flex; -` - const Content = () => <> export const Body = styled.div` @@ -102,7 +84,7 @@ export const Info = styled.div` align-items: center; ` -export const Title = styled.span` +export const Title = styled(Linkable)` flex-grow: 1; color: ${theme.custom.colors.darkGray2}; text-overflow: ellipsis; @@ -170,6 +152,7 @@ type CardProps = { className?: string href?: string draggable?: boolean + onClick?: () => void } export type Card = FC & { Content: FC<{ children: ReactNode }> @@ -181,15 +164,13 @@ export type Card = FC & { Action: FC } -const ListCard: Card = ({ children, className, href, draggable }) => { - const _Container = draggable - ? DraggableContainer - : href - ? LinkContainer - : Container - +const ListCard: Card = ({ children, className, href, draggable, onClick }) => { let content, imageProps, info, title, footer, actions + const hasHref = typeof href === "string" + const handleHrefClick = useClickChildHref(href, onClick) + const handleClick = hasHref ? handleHrefClick : onClick + Children.forEach(children, (child) => { if (!isValidElement(child)) return if (child.type === Content) content = child.props.children @@ -203,15 +184,15 @@ const ListCard: Card = ({ children, className, href, draggable }) => { const classNames = ["MitListCard-root", className ?? ""].join(" ") if (content) { return ( - <_Container className={classNames} href={href!}> + {content} - + ) } return ( - <_Container href={href!} scroll={!href?.startsWith("?")}> + {draggable && ( @@ -219,7 +200,7 @@ const ListCard: Card = ({ children, className, href, draggable }) => { )} {info} - + <Title href={href}> <TruncateText lineClamp={2}>{title}</TruncateText> @@ -231,7 +212,7 @@ const ListCard: Card = ({ children, className, href, draggable }) => { // eslint-disable-next-line styled-components-a11y/alt-text )} - + {actions && {actions}} ) diff --git a/frontends/ol-components/src/components/Card/ListCardCondensed.tsx b/frontends/ol-components/src/components/Card/ListCardCondensed.tsx index 58a49dfec5..73f80600d8 100644 --- a/frontends/ol-components/src/components/Card/ListCardCondensed.tsx +++ b/frontends/ol-components/src/components/Card/ListCardCondensed.tsx @@ -2,14 +2,11 @@ import React, { FC, ReactNode, Children, isValidElement } from "react" import styled from "@emotion/styled" import { RiDraggable } from "@remixicon/react" import { theme } from "../ThemeProvider/ThemeProvider" -import { Wrapper } from "./Card" +import { Wrapper, Container, useClickChildHref } from "./Card" import { TruncateText } from "../TruncateText/TruncateText" import { ListCard, Body as BaseBody, - LinkContainer, - Container, - DraggableContainer, DragArea as BaseDragArea, Info as BaseInfo, Title as BaseTitle, @@ -73,19 +70,24 @@ type CardProps = { className?: string href?: string draggable?: boolean + onClick?: () => void } type Card = FC & Omit -const ListCardCondensed: Card = ({ children, className, href, draggable }) => { - const _Container = draggable - ? DraggableContainer - : href - ? LinkContainer - : Container - +const ListCardCondensed: Card = ({ + children, + className, + href, + draggable, + onClick, +}) => { let content, info, title, footer, actions + const hasHref = typeof href === "string" + const handleHrefClick = useClickChildHref(href, onClick) + const handleClick = hasHref ? handleHrefClick : onClick + Children.forEach(children, (child) => { if (!isValidElement(child)) return if (child.type === Content) content = child.props.children @@ -97,15 +99,15 @@ const ListCardCondensed: Card = ({ children, className, href, draggable }) => { if (content) { return ( - <_Container className={className} href={href!}> + {content} - + ) } return ( - <_Container href={href!} scroll={!href?.startsWith("?")}> + {draggable && ( @@ -113,14 +115,14 @@ const ListCardCondensed: Card = ({ children, className, href, draggable }) => { )} {info} - + <Title href={href}> <TruncateText lineClamp={4}>{title}</TruncateText>
{footer}
 - + {actions && {actions}} ) From 54efcef3c9b770467ab0751a1be1af4ee80f3052 Mon Sep 17 00:00:00 2001 From: Chris Chudzicki Date: Mon, 28 Oct 2024 17:04:21 -0400 Subject: [PATCH 03/10] add resourceType label to resource cards --- .../src/components/Card/Card.tsx | 14 ++++++++-- .../src/components/Card/ListCard.tsx | 28 +++++++++++++------ .../LearningResourceCard.tsx | 5 ++-- .../LearningResourceListCard.tsx | 7 +++-- 4 files changed, 39 insertions(+), 15 deletions(-) diff --git a/frontends/ol-components/src/components/Card/Card.tsx b/frontends/ol-components/src/components/Card/Card.tsx index e1824c7a6a..ff545d6686 100644 --- a/frontends/ol-components/src/components/Card/Card.tsx +++ b/frontends/ol-components/src/components/Card/Card.tsx @@ -5,6 +5,7 @@ import React, { isValidElement, CSSProperties, useCallback, + AriaAttributes, } from "react" import styled from "@emotion/styled" import { theme } from "../ThemeProvider/ThemeProvider" @@ -18,7 +19,7 @@ type LinkableProps = { href?: string children?: ReactNode className?: string -} +} & Pick /** * Render a NextJS link if href is provided, otherwise a span. * Does not scroll if the href is a query string. @@ -27,10 +28,16 @@ export const Linkable: React.FC = ({ href, children, className, + "aria-label": ariaLabel, }) => { if (href) { return ( - + {children} ) @@ -206,7 +213,8 @@ type TitleProps = { children?: ReactNode lines?: number style?: CSSProperties -} +} & Pick + type SlotProps = { children?: ReactNode; style?: CSSProperties } type Card = FC & { diff --git a/frontends/ol-components/src/components/Card/ListCard.tsx b/frontends/ol-components/src/components/Card/ListCard.tsx index 04ecbfdadc..2998028991 100644 --- a/frontends/ol-components/src/components/Card/ListCard.tsx +++ b/frontends/ol-components/src/components/Card/ListCard.tsx @@ -1,4 +1,10 @@ -import React, { FC, ReactNode, Children, isValidElement } from "react" +import React, { + FC, + ReactNode, + Children, + isValidElement, + AriaAttributes, +} from "react" import styled from "@emotion/styled" import { RiDraggable } from "@remixicon/react" import { theme } from "../ThemeProvider/ThemeProvider" @@ -154,19 +160,23 @@ type CardProps = { draggable?: boolean onClick?: () => void } +type TitleProps = { + children?: ReactNode +} & Pick + export type Card = FC & { Content: FC<{ children: ReactNode }> Image: FC Info: FC<{ children: ReactNode }> - Title: FC<{ children: ReactNode }> + Title: FC Footer: FC<{ children: ReactNode }> Actions: FC<{ children: ReactNode }> Action: FC } const ListCard: Card = ({ children, className, href, draggable, onClick }) => { - let content, imageProps, info, title, footer, actions - + let content, imageProps, info, footer, actions + let title: TitleProps = {} const hasHref = typeof href === "string" const handleHrefClick = useClickChildHref(href, onClick) const handleClick = hasHref ? handleHrefClick : onClick @@ -176,7 +186,7 @@ const ListCard: Card = ({ children, className, href, draggable, onClick }) => { if (child.type === Content) content = child.props.children else if (child.type === Image) imageProps = child.props else if (child.type === Info) info = child.props.children - else if (child.type === Title) title = child.props.children + else if (child.type === Title) title = child.props else if (child.type === Footer) footer = child.props.children else if (child.type === Actions) actions = child.props.children }) @@ -200,9 +210,11 @@ const ListCard: Card = ({ children, className, href, draggable, onClick }) => { )} {info} - - <TruncateText lineClamp={2}>{title}</TruncateText> - + {title && ( + + <TruncateText lineClamp={2}>{title.children}</TruncateText> + + )}
{footer}
 diff --git a/frontends/ol-components/src/components/LearningResourceCard/LearningResourceCard.tsx b/frontends/ol-components/src/components/LearningResourceCard/LearningResourceCard.tsx index 314d2b8912..ca7acb0c26 100644 --- a/frontends/ol-components/src/components/LearningResourceCard/LearningResourceCard.tsx +++ b/frontends/ol-components/src/components/LearningResourceCard/LearningResourceCard.tsx @@ -229,6 +229,7 @@ const LearningResourceCard: React.FC = ({ return null } + const readableType = getReadableResourceType(resource.resource_type) return ( = ({ - + {resource.title} @@ -257,7 +258,7 @@ const LearningResourceCard: React.FC = ({ {onAddToUserListClick && ( onAddToUserListClick(event, resource.id)} > {inUserList ? ( diff --git a/frontends/ol-components/src/components/LearningResourceCard/LearningResourceListCard.tsx b/frontends/ol-components/src/components/LearningResourceCard/LearningResourceListCard.tsx index 6778afc08b..9e31174bb8 100644 --- a/frontends/ol-components/src/components/LearningResourceCard/LearningResourceListCard.tsx +++ b/frontends/ol-components/src/components/LearningResourceCard/LearningResourceListCard.tsx @@ -307,6 +307,7 @@ const LearningResourceListCard: React.FC = ({ if (!resource) { return null } + const readableType = getReadableResourceType(resource.resource_type) return ( = ({ - {resource.title} + + {resource.title} + {onAddToLearningPathClick && ( = ({ {onAddToUserListClick && ( onAddToUserListClick(event, resource.id)} > {inUserList ? ( From 95842bacb53edb7cc3314ddf70ac10d33e14659f Mon Sep 17 00:00:00 2001 From: Chris Chudzicki Date: Tue, 29 Oct 2024 09:54:05 -0400 Subject: [PATCH 04/10] dont clip title outline --- frontends/ol-components/src/components/Card/Card.tsx | 1 + frontends/ol-components/src/components/Card/ListCard.tsx | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/frontends/ol-components/src/components/Card/Card.tsx b/frontends/ol-components/src/components/Card/Card.tsx index ff545d6686..fd8e629dad 100644 --- a/frontends/ol-components/src/components/Card/Card.tsx +++ b/frontends/ol-components/src/components/Card/Card.tsx @@ -72,6 +72,7 @@ export const Container = styled.div<{ display?: CSSProperties["display"] }>( background: theme.custom.colors.white, display, position: "relative", + overflow: "hidden", // to clip image so they match border radius }, onClick && { "&:hover": { diff --git a/frontends/ol-components/src/components/Card/ListCard.tsx b/frontends/ol-components/src/components/Card/ListCard.tsx index 2998028991..76359c0c43 100644 --- a/frontends/ol-components/src/components/Card/ListCard.tsx +++ b/frontends/ol-components/src/components/Card/ListCard.tsx @@ -23,7 +23,6 @@ const Content = () => <> export const Body = styled.div` flex-grow: 1; - overflow: hidden; margin: 24px; ${theme.breakpoints.down("md")} { margin: 12px; From f83192addd6677ff3a9f10fdc6e4f5b529ab234e Mon Sep 17 00:00:00 2001 From: Chris Chudzicki Date: Tue, 29 Oct 2024 10:57:18 -0400 Subject: [PATCH 05/10] tweak two tests --- .../LearningResourceCard/LearningResourceCard.test.tsx | 5 ++++- .../LearningResourceListCard.test.tsx | 9 ++++++--- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/frontends/ol-components/src/components/LearningResourceCard/LearningResourceCard.test.tsx b/frontends/ol-components/src/components/LearningResourceCard/LearningResourceCard.test.tsx index 9659c4812b..02878fff8a 100644 --- a/frontends/ol-components/src/components/LearningResourceCard/LearningResourceCard.test.tsx +++ b/frontends/ol-components/src/components/LearningResourceCard/LearningResourceCard.test.tsx @@ -96,8 +96,11 @@ describe("Learning Resource Card", () => { setup({ resource, href: "/path/to/thing" }) const link = screen.getByRole("link", { - name: new RegExp(resource.title), + // Accessible name has resource type prepended + name: `Course: ${resource.title}`, }) + // Visual title does not have resource name prepended + expect(link.textContent).toBe(resource.title) expect(new URL(link.href).pathname).toBe("/path/to/thing") }) diff --git a/frontends/ol-components/src/components/LearningResourceCard/LearningResourceListCard.test.tsx b/frontends/ol-components/src/components/LearningResourceCard/LearningResourceListCard.test.tsx index d90eae55ae..d3df84df46 100644 --- a/frontends/ol-components/src/components/LearningResourceCard/LearningResourceListCard.test.tsx +++ b/frontends/ol-components/src/components/LearningResourceCard/LearningResourceListCard.test.tsx @@ -92,11 +92,14 @@ describe("Learning Resource List Card", () => { setup({ resource, href: "/path/to/thing" }) - const card = screen.getByRole("link", { - name: new RegExp(resource.title), + const link = screen.getByRole("link", { + // Accessible name has resource type prepended + name: `Course: ${resource.title}`, }) + // Visual title does not have resource name prepended + expect(link.textContent).toBe(resource.title) - expect(card).toHaveAttribute("href", "/path/to/thing") + expect(link).toHaveAttribute("href", "/path/to/thing") }) test("Click action buttons", async () => { From 5d198bce406263a2f40b1a7314a70dcb320f5eb1 Mon Sep 17 00:00:00 2001 From: Chris Chudzicki Date: Tue, 29 Oct 2024 11:45:16 -0400 Subject: [PATCH 06/10] remove absolute positioning from Card --- .../src/components/Card/Card.tsx | 101 ++++++++---------- .../src/components/Card/ListCard.tsx | 10 +- .../src/components/Card/ListCardCondensed.tsx | 10 +- 3 files changed, 57 insertions(+), 64 deletions(-) diff --git a/frontends/ol-components/src/components/Card/Card.tsx b/frontends/ol-components/src/components/Card/Card.tsx index fd8e629dad..3ca0aeee63 100644 --- a/frontends/ol-components/src/components/Card/Card.tsx +++ b/frontends/ol-components/src/components/Card/Card.tsx @@ -50,21 +50,11 @@ export const Linkable: React.FC = ({ * Link container in the DOM structure. They cannot be a descendent of it as * buttons inside anchors are not valid HTML. */ -export const Wrapper = styled.div<{ size?: Size }>` +export const Wrapper = styled.div` position: relative; - ${({ size }) => { - let width - if (!size) return "" - if (size === "medium") width = 300 - if (size === "small") width = 192 - return ` - min-width: ${width}px; - max-width: ${width}px; - ` - }} ` -export const Container = styled.div<{ display?: CSSProperties["display"] }>( +export const BaseContainer = styled.div<{ display?: CSSProperties["display"] }>( ({ theme, onClick, display = "block" }) => [ { borderRadius: "8px", @@ -84,6 +74,16 @@ export const Container = styled.div<{ display?: CSSProperties["display"] }>( }, ], ) +const CONTAINER_WIDTHS: Record = { + small: 192, + medium: 300, +} +const Container = styled(BaseContainer)<{ size?: Size }>(({ size }) => [ + size && { + minWidth: CONTAINER_WIDTHS[size], + maxWidth: CONTAINER_WIDTHS[size], + }, +]) const Content = () => <> @@ -155,9 +155,6 @@ const Bottom = styled.div` const Actions = styled.div` display: flex; gap: 8px; - position: absolute; - bottom: 16px; - right: 16px; ` /** @@ -266,50 +263,46 @@ const Card: Card = ({ children, className, size, href, onClick }) => { if (content) { return ( - - - {content} - - + + {content} + ) } return ( - - - {image && ( - // alt text will be checked on Card.Image - // eslint-disable-next-line styled-components-a11y/alt-text - - )} - - {info.children && ( - - {info.children} - - )} - - {title.children} - - - -
- {footer.children} -
- - - {actions.children && ( - - {actions.children} - + + {image && ( + // alt text will be checked on Card.Image + // eslint-disable-next-line styled-components-a11y/alt-text + )} - + + {info.children && ( + + {info.children} + + )} + + {title.children} + + + +
+ {footer.children} +
+ {actions.children && ( + + {actions.children} + + )} + + ) } diff --git a/frontends/ol-components/src/components/Card/ListCard.tsx b/frontends/ol-components/src/components/Card/ListCard.tsx index 76359c0c43..cc53e64bde 100644 --- a/frontends/ol-components/src/components/Card/ListCard.tsx +++ b/frontends/ol-components/src/components/Card/ListCard.tsx @@ -10,7 +10,7 @@ import { RiDraggable } from "@remixicon/react" import { theme } from "../ThemeProvider/ThemeProvider" import { Wrapper, - Container, + BaseContainer, ImageProps, useClickChildHref, Linkable, @@ -193,15 +193,15 @@ const ListCard: Card = ({ children, className, href, draggable, onClick }) => { const classNames = ["MitListCard-root", className ?? ""].join(" ") if (content) { return ( - + {content} - + ) } return ( - + {draggable && ( @@ -223,7 +223,7 @@ const ListCard: Card = ({ children, className, href, draggable, onClick }) => { // eslint-disable-next-line styled-components-a11y/alt-text )} - + {actions && {actions}} ) diff --git a/frontends/ol-components/src/components/Card/ListCardCondensed.tsx b/frontends/ol-components/src/components/Card/ListCardCondensed.tsx index 73f80600d8..59cbaaa949 100644 --- a/frontends/ol-components/src/components/Card/ListCardCondensed.tsx +++ b/frontends/ol-components/src/components/Card/ListCardCondensed.tsx @@ -2,7 +2,7 @@ import React, { FC, ReactNode, Children, isValidElement } from "react" import styled from "@emotion/styled" import { RiDraggable } from "@remixicon/react" import { theme } from "../ThemeProvider/ThemeProvider" -import { Wrapper, Container, useClickChildHref } from "./Card" +import { Wrapper, BaseContainer, useClickChildHref } from "./Card" import { TruncateText } from "../TruncateText/TruncateText" import { ListCard, @@ -99,15 +99,15 @@ const ListCardCondensed: Card = ({ if (content) { return ( - + {content} - + ) } return ( - + {draggable && ( @@ -122,7 +122,7 @@ const ListCardCondensed: Card = ({
{footer}
- + {actions && {actions}} ) From ac6c55f5f2c421d888a906b21a51d3aa2798ec53 Mon Sep 17 00:00:00 2001 From: Chris Chudzicki Date: Tue, 29 Oct 2024 13:41:25 -0400 Subject: [PATCH 07/10] remove absolute positioning from ListCard etc --- .../src/components/Card/ListCard.tsx | 65 +++++++------------ .../src/components/Card/ListCardCondensed.tsx | 49 ++++++-------- 2 files changed, 45 insertions(+), 69 deletions(-) diff --git a/frontends/ol-components/src/components/Card/ListCard.tsx b/frontends/ol-components/src/components/Card/ListCard.tsx index cc53e64bde..ac93b2484b 100644 --- a/frontends/ol-components/src/components/Card/ListCard.tsx +++ b/frontends/ol-components/src/components/Card/ListCard.tsx @@ -8,13 +8,7 @@ import React, { import styled from "@emotion/styled" import { RiDraggable } from "@remixicon/react" import { theme } from "../ThemeProvider/ThemeProvider" -import { - Wrapper, - BaseContainer, - ImageProps, - useClickChildHref, - Linkable, -} from "./Card" +import { BaseContainer, ImageProps, useClickChildHref, Linkable } from "./Card" import { TruncateText } from "../TruncateText/TruncateText" import { ActionButton, ActionButtonProps } from "../Button/Button" import { default as NextImage } from "next/image" @@ -123,19 +117,12 @@ export const Bottom = styled.div` /** * Slot intended to contain ListCardAction buttons. */ -export const Actions = styled.div<{ hasImage?: boolean }>` +export const Actions = styled.div` display: flex; gap: 8px; - position: absolute; - bottom: 24px; - right: ${({ hasImage }) => (hasImage ? "284px" : "24px")}; ${theme.breakpoints.down("md")} { - bottom: 8px; gap: 4px; - right: ${({ hasImage }) => (hasImage ? "120px" : "8px")}; } - - background-color: ${theme.custom.colors.white}; ` const ListCardActionButton = styled(ActionButton)<{ isMobile?: boolean }>( @@ -200,32 +187,30 @@ const ListCard: Card = ({ children, className, href, draggable, onClick }) => { } return ( - - - {draggable && ( - - - - )} - - {info} - {title && ( - - <TruncateText lineClamp={2}>{title.children}</TruncateText> - - )} - -
{footer}
- - - {imageProps && ( - // alt text will be checked on ListCard.Image - // eslint-disable-next-line styled-components-a11y/alt-text - + + {draggable && ( + + + + )} + + {info} + {title && ( + + <TruncateText lineClamp={2}>{title.children}</TruncateText> + )} - - {actions && {actions}} - + +
{footer}
+ {actions && {actions}} + + + {imageProps && ( + // alt text will be checked on ListCard.Image + // eslint-disable-next-line styled-components-a11y/alt-text + + )} + ) } diff --git a/frontends/ol-components/src/components/Card/ListCardCondensed.tsx b/frontends/ol-components/src/components/Card/ListCardCondensed.tsx index 59cbaaa949..5ff677e587 100644 --- a/frontends/ol-components/src/components/Card/ListCardCondensed.tsx +++ b/frontends/ol-components/src/components/Card/ListCardCondensed.tsx @@ -2,7 +2,7 @@ import React, { FC, ReactNode, Children, isValidElement } from "react" import styled from "@emotion/styled" import { RiDraggable } from "@remixicon/react" import { theme } from "../ThemeProvider/ThemeProvider" -import { Wrapper, BaseContainer, useClickChildHref } from "./Card" +import { BaseContainer, useClickChildHref } from "./Card" import { TruncateText } from "../TruncateText/TruncateText" import { ListCard, @@ -11,7 +11,6 @@ import { Info as BaseInfo, Title as BaseTitle, Footer, - Actions as BaseActions, Bottom as BaseBottom, } from "./ListCard" import type { Card as BaseCard } from "./ListCard" @@ -53,15 +52,9 @@ const Bottom = styled(BaseBottom)` height: auto; } ` -const Actions = styled(BaseActions)` - bottom: 16px; - right: 16px; +const Actions = styled.div` + display: flex; gap: 16px; - ${theme.breakpoints.down("md")} { - bottom: 16px; - right: 16px; - gap: 16px; - } ` const Content = () => <> @@ -106,25 +99,23 @@ const ListCardCondensed: Card = ({ } return ( - - - {draggable && ( - - - - )} - - {info} - - <TruncateText lineClamp={4}>{title}</TruncateText> - - -
{footer}
- - - - {actions && {actions}} - + + {draggable && ( + + + + )} + + {info} + + <TruncateText lineClamp={4}>{title}</TruncateText> + + +
{footer}
+ {actions && {actions}} + + + ) } From b8dfa2c19cca992c3dbe2a26af7f440c27935a16 Mon Sep 17 00:00:00 2001 From: Chris Chudzicki Date: Tue, 29 Oct 2024 20:21:34 -0400 Subject: [PATCH 08/10] Make link forwarding opt-in; LR cards articles --- .../app-pages/HomePage/NewsEventsSection.tsx | 4 +- .../ConfiguredPostHogProvider.tsx | 4 + .../src/components/Card/Card.test.tsx | 116 ++++++++++++------ .../src/components/Card/Card.tsx | 78 +++++++----- .../src/components/Card/ListCard.test.tsx | 114 +++++++++++------ .../src/components/Card/ListCard.tsx | 45 +++++-- .../src/components/Card/ListCardCondensed.tsx | 38 +++++- .../LearningResourceCard.test.tsx | 43 ++++--- .../LearningResourceCard.tsx | 11 +- .../LearningResourceListCard.test.tsx | 39 +++--- .../LearningResourceListCard.tsx | 13 +- .../LearningResourceListCardCondensed.tsx | 12 +- 12 files changed, 362 insertions(+), 155 deletions(-) diff --git a/frontends/main/src/app-pages/HomePage/NewsEventsSection.tsx b/frontends/main/src/app-pages/HomePage/NewsEventsSection.tsx index cffb7c7e4d..2a5939b82d 100644 --- a/frontends/main/src/app-pages/HomePage/NewsEventsSection.tsx +++ b/frontends/main/src/app-pages/HomePage/NewsEventsSection.tsx @@ -188,7 +188,7 @@ const Story: React.FC<{ item: NewsFeedItem; mobile: boolean }> = ({ mobile, }) => { return ( - + {item.image.url ? ( ) : null} @@ -222,7 +222,7 @@ const NewsEventsSection: React.FC = () => { const stories = news.results.slice(0, 6) const EventCards = events.results.map((item) => ( - + diff --git a/frontends/main/src/components/ConfiguredPostHogProvider/ConfiguredPostHogProvider.tsx b/frontends/main/src/components/ConfiguredPostHogProvider/ConfiguredPostHogProvider.tsx index e69b9afc5d..475af24453 100644 --- a/frontends/main/src/components/ConfiguredPostHogProvider/ConfiguredPostHogProvider.tsx +++ b/frontends/main/src/components/ConfiguredPostHogProvider/ConfiguredPostHogProvider.tsx @@ -16,6 +16,10 @@ const ConfiguredPostHogProvider: React.FC<{ children: React.ReactNode }> = ({ }, } + console.log({ + postHogOptions, + }) + return apiKey ? ( {children} diff --git a/frontends/ol-components/src/components/Card/Card.test.tsx b/frontends/ol-components/src/components/Card/Card.test.tsx index e29f5490f3..d0b9827580 100644 --- a/frontends/ol-components/src/components/Card/Card.test.tsx +++ b/frontends/ol-components/src/components/Card/Card.test.tsx @@ -5,7 +5,6 @@ import React from "react" import { getOriginalSrc } from "ol-test-utilities" import invariant from "tiny-invariant" import { ThemeProvider } from "../ThemeProvider/ThemeProvider" -import { faker } from "@faker-js/faker/locale/en" describe("Card", () => { test("has class MitCard-root on root element", () => { @@ -42,49 +41,96 @@ describe("Card", () => { expect(actions).toHaveTextContent("Actions") }) - test("The whole card is clickable as a link", async () => { - const href = `#${faker.lorem.word()}` + test.each([ + { forwardClicksToLink: true, finalHref: "#woof" }, + { forwardClicksToLink: false, finalHref: "" }, + ])( + "The whole card is clickable as a link if forwardClicksToLink ($forwardClicksToLink)", + async ({ forwardClicksToLink, finalHref }) => { + const href = "#woof" + render( + + Title + + Info + Footer + Actions + , + { wrapper: ThemeProvider }, + ) + const card = document.querySelector(".MitCard-root") + invariant(card instanceof HTMLDivElement) // Sanity: Chceck it's not an anchor + + await user.click(card) + expect(window.location.hash).toBe(finalHref) + }, + ) + + test.each([ + { forwardClicksToLink: true, finalHref: "#meow" }, + { forwardClicksToLink: false, finalHref: "" }, + ])( + "The whole card is clickable as a link when using Content when forwardClicksToLink ($forwardClicksToLink), except buttons and links", + async ({ finalHref, forwardClicksToLink }) => { + const href = "#meow" + const onClick = jest.fn() + render( + + +
Hello!
+
+ +
+ Link + + , + { wrapper: ThemeProvider }, + ) + const button = screen.getByRole("button", { name: "Button" }) + await user.click(button) + expect(onClick).toHaveBeenCalled() + expect(window.location.hash).toBe("") + + // outermost wrapper is not actually clickable + const card = document.querySelector(".MitCard-root") + invariant(card instanceof HTMLDivElement) // Sanity: Chceck it's not an anchor + + await user.click(card) + expect(window.location.hash).toBe(finalHref) + }, + ) + + test("Clicks on interactive elements are not forwarded", async () => { + const btnOnClick = jest.fn() + const divOnClick = jest.fn() render( - + Title Info - Footer - Actions - , - { wrapper: ThemeProvider }, - ) - // outermost wrapper is not actually clickable - const card = document.querySelector(".MitCard-root > *") - invariant(card instanceof HTMLDivElement) // Sanity: Chceck it's not an anchor - - await user.click(card) - expect(window.location.hash).toBe(href) - }) - - test("The whole card is clickable as a link when using Content, except buttons and links", async () => { - const href = `#${faker.lorem.word()}` - const onClick = jest.fn() - render( - - -
Hello!
- - Link - + + + Link Two + {/* + eslint-disable-next-line jsx-a11y/click-events-have-key-events, jsx-a11y/no-static-element-interactions + */} +
+ Interactive Div +
+ , { wrapper: ThemeProvider }, ) const button = screen.getByRole("button", { name: "Button" }) + const link = screen.getByRole("link", { name: "Link Two" }) + const div = screen.getByText("Interactive Div") await user.click(button) - expect(onClick).toHaveBeenCalled() + expect(btnOnClick).toHaveBeenCalled() expect(window.location.hash).toBe("") - - // outermost wrapper is not actually clickable - const card = document.querySelector(".MitCard-root > *") - invariant(card instanceof HTMLDivElement) // Sanity: Chceck it's not an anchor - - await user.click(card) - expect(window.location.hash).toBe(href) + await user.click(link) + expect(window.location.hash).toBe("#two") + await user.click(div) + expect(divOnClick).toHaveBeenCalled() + expect(window.location.hash).toBe("#two") }) }) diff --git a/frontends/ol-components/src/components/Card/Card.tsx b/frontends/ol-components/src/components/Card/Card.tsx index 3ca0aeee63..b69284a8d4 100644 --- a/frontends/ol-components/src/components/Card/Card.tsx +++ b/frontends/ol-components/src/components/Card/Card.tsx @@ -19,7 +19,7 @@ type LinkableProps = { href?: string children?: ReactNode className?: string -} & Pick +} /** * Render a NextJS link if href is provided, otherwise a span. * Does not scroll if the href is a query string. @@ -28,16 +28,10 @@ export const Linkable: React.FC = ({ href, children, className, - "aria-label": ariaLabel, }) => { if (href) { return ( - + {children} ) @@ -45,15 +39,6 @@ export const Linkable: React.FC = ({ return {children} } -/* - *The relative positioned wrapper allows the action buttons to live adjacent to the - * Link container in the DOM structure. They cannot be a descendent of it as - * buttons inside anchors are not valid HTML. - */ -export const Wrapper = styled.div` - position: relative; -` - export const BaseContainer = styled.div<{ display?: CSSProperties["display"] }>( ({ theme, onClick, display = "block" }) => [ { @@ -61,7 +46,6 @@ export const BaseContainer = styled.div<{ display?: CSSProperties["display"] }>( border: `1px solid ${theme.custom.colors.lightGray2}`, background: theme.custom.colors.white, display, - position: "relative", overflow: "hidden", // to clip image so they match border radius }, onClick && { @@ -170,11 +154,17 @@ export const useClickChildHref = ( return useCallback( (e) => { onClick?.(e) + if (!e.currentTarget.contains(e.target as Node)) { + // This happens if click target is a child in React tree but not DOM tree + // This can happen with portals. + // In such cases, data-card-actions won't be a parent of the target. + return + } const anchor = e.currentTarget.querySelector( `a[href="${href}"]`, ) const target = e.target as HTMLElement - if (!anchor || target.closest("a, button")) return + if (!anchor || target.closest("a, button, [data-card-action]")) return if (e.metaKey || e.ctrlKey) { const opts: PointerEventInit = { bubbles: false, @@ -195,12 +185,26 @@ type CardProps = { className?: string size?: Size /** - * If provided, the card will render its title as a link. The entire card will - * be clickable, activating the link. + * If provided, the card will render its title as a link. + * + * Clicks on the entire card can be forwarded to the link via `forwardClicksToLink`. */ href?: string + /** + * Defaults to `false`. If `true`, clicking the whole card will click the + * href link as well. + * + * NOTES: + * - If using Card.Content to customize, you must ensure the content includes + * an anchor with the card's href. + * - Clicks will NOT be forwarded if it is (or is a child of): + * - an anchor or button element + * - OR an element with data-card-action + */ + forwardClicksToLink?: boolean onClick?: React.MouseEventHandler -} + as?: React.ElementType +} & AriaAttributes export type ImageProps = NextImageProps & { size?: Size @@ -211,7 +215,7 @@ type TitleProps = { children?: ReactNode lines?: number style?: CSSProperties -} & Pick +} type SlotProps = { children?: ReactNode; style?: CSSProperties } @@ -224,7 +228,15 @@ type Card = FC & { Actions: FC } -const Card: Card = ({ children, className, size, href, onClick }) => { +const Card: Card = ({ + children, + className, + size, + href, + onClick, + forwardClicksToLink = false, + ...others +}) => { let content, image: ImageProps | null = null, info: SlotProps = {}, @@ -257,20 +269,30 @@ const Card: Card = ({ children, className, size, href, onClick }) => { const hasHref = typeof href === "string" const handleHrefClick = useClickChildHref(href, onClick) - const handleClick = hasHref ? handleHrefClick : onClick + const handleClick = hasHref && forwardClicksToLink ? handleHrefClick : onClick const allClassNames = ["MitCard-root", className ?? ""].join(" ") if (content) { return ( - + {content} ) } return ( - + {image && ( // alt text will be checked on Card.Image // eslint-disable-next-line styled-components-a11y/alt-text @@ -297,7 +319,7 @@ const Card: Card = ({ children, className, size, href, onClick }) => { {footer.children}
{actions.children && ( - + {actions.children} )} diff --git a/frontends/ol-components/src/components/Card/ListCard.test.tsx b/frontends/ol-components/src/components/Card/ListCard.test.tsx index 98d26f345a..bf18d6385a 100644 --- a/frontends/ol-components/src/components/Card/ListCard.test.tsx +++ b/frontends/ol-components/src/components/Card/ListCard.test.tsx @@ -2,7 +2,6 @@ import { render, screen } from "@testing-library/react" import user from "@testing-library/user-event" import { ListCard } from "./ListCard" import React from "react" -import { faker } from "@faker-js/faker/locale/en" import invariant from "tiny-invariant" import { ThemeProvider } from "../ThemeProvider/ThemeProvider" @@ -20,48 +19,93 @@ describe("ListCard", () => { expect(card).toHaveClass("Foo") }) - test("The whole card is clickable as a link", async () => { - const href = `#${faker.lorem.word()}` - render( - - Title - Info - Footer - Actions - , - { wrapper: ThemeProvider }, - ) - // outermost wrapper is not actually clickable - const card = document.querySelector(".MitListCard-root > *") - invariant(card instanceof HTMLDivElement) // Sanity: Chceck it's not an anchor + test.each([ + { forwardClicksToLink: true, finalHref: "#woof" }, + { forwardClicksToLink: false, finalHref: "" }, + ])( + "The whole card is clickable as a link", + async ({ forwardClicksToLink, finalHref }) => { + const href = "#woof" + render( + + Title + Info + Footer + Actions + , + { wrapper: ThemeProvider }, + ) + // outermost wrapper is not actually clickable + const card = document.querySelector(".MitListCard-root > *") + invariant(card instanceof HTMLDivElement) // Sanity: Chceck it's not an anchor - await user.click(card) - expect(window.location.hash).toBe(href) - }) + await user.click(card) + expect(window.location.hash).toBe(finalHref) + }, + ) - test("The whole card is clickable as a link when using Content, except buttons and links", async () => { - const href = `#${faker.lorem.word()}` - const onClick = jest.fn() + test.each([ + { forwardClicksToLink: true, finalHref: "#meow" }, + { forwardClicksToLink: false, finalHref: "" }, + ])( + "The whole card is clickable as a link when using Content, except buttons and links", + async ({ forwardClicksToLink, finalHref }) => { + const href = "#meow" + const onClick = jest.fn() + render( + + +
Hello!
+ + Link + + , + { wrapper: ThemeProvider }, + ) + const button = screen.getByRole("button", { name: "Button" }) + await user.click(button) + expect(onClick).toHaveBeenCalled() + expect(window.location.hash).toBe("") + + // outermost wrapper is not actually clickable + const card = document.querySelector(".MitListCard-root > *") + invariant(card instanceof HTMLDivElement) // Sanity: Chceck it's not an anchor + + await user.click(card) + expect(window.location.hash).toBe(finalHref) + }, + ) + + test("Clicks on interactive elements are not forwarded", async () => { + const btnOnClick = jest.fn() + const divOnClick = jest.fn() render( - - -
Hello!
- - Link - + + Title + Info + + + Link Two + {/* + eslint-disable-next-line jsx-a11y/click-events-have-key-events, jsx-a11y/no-static-element-interactions + */} +
+ Interactive Div +
+ , { wrapper: ThemeProvider }, ) const button = screen.getByRole("button", { name: "Button" }) + const link = screen.getByRole("link", { name: "Link Two" }) + const div = screen.getByText("Interactive Div") await user.click(button) - expect(onClick).toHaveBeenCalled() + expect(btnOnClick).toHaveBeenCalled() expect(window.location.hash).toBe("") - - // outermost wrapper is not actually clickable - const card = document.querySelector(".MitListCard-root > *") - invariant(card instanceof HTMLDivElement) // Sanity: Chceck it's not an anchor - - await user.click(card) - expect(window.location.hash).toBe(href) + await user.click(link) + expect(window.location.hash).toBe("#two") + await user.click(div) + expect(divOnClick).toHaveBeenCalled() + expect(window.location.hash).toBe("#two") }) }) diff --git a/frontends/ol-components/src/components/Card/ListCard.tsx b/frontends/ol-components/src/components/Card/ListCard.tsx index ac93b2484b..31f44e5079 100644 --- a/frontends/ol-components/src/components/Card/ListCard.tsx +++ b/frontends/ol-components/src/components/Card/ListCard.tsx @@ -142,13 +142,31 @@ const ListCardActionButton = styled(ActionButton)<{ isMobile?: boolean }>( type CardProps = { children: ReactNode[] | ReactNode className?: string + /** + * If provided, the card will render its title as a link. + * + * Clicks on the entire card can be forwarded to the link via `forwardClicksToLink`. + */ href?: string + /** + * Defaults to `false`. If `true`, clicking the whole card will click the + * href link as well. + * + * NOTES: + * - If using Card.Content to customize, you must ensure the content includes + * an anchor with the card's href. + * - Clicks will NOT be forwarded if: + * - The click target is a child of Card.Actions OR an element with + * - The click target is a child of any element with data-card-actions attribute + */ + forwardClicksToLink?: boolean draggable?: boolean onClick?: () => void -} + as?: React.ElementType +} & AriaAttributes type TitleProps = { children?: ReactNode -} & Pick +} export type Card = FC & { Content: FC<{ children: ReactNode }> @@ -160,12 +178,20 @@ export type Card = FC & { Action: FC } -const ListCard: Card = ({ children, className, href, draggable, onClick }) => { +const ListCard: Card = ({ + children, + className, + href, + forwardClicksToLink = false, + draggable, + onClick, + ...others +}) => { let content, imageProps, info, footer, actions let title: TitleProps = {} const hasHref = typeof href === "string" const handleHrefClick = useClickChildHref(href, onClick) - const handleClick = hasHref ? handleHrefClick : onClick + const handleClick = hasHref && forwardClicksToLink ? handleHrefClick : onClick Children.forEach(children, (child) => { if (!isValidElement(child)) return @@ -180,14 +206,19 @@ const ListCard: Card = ({ children, className, href, draggable, onClick }) => { const classNames = ["MitListCard-root", className ?? ""].join(" ") if (content) { return ( - + {content} ) } return ( - + {draggable && ( @@ -202,7 +233,7 @@ const ListCard: Card = ({ children, className, href, draggable, onClick }) => { )}
{footer}
- {actions && {actions}} + {actions && {actions}} {imageProps && ( diff --git a/frontends/ol-components/src/components/Card/ListCardCondensed.tsx b/frontends/ol-components/src/components/Card/ListCardCondensed.tsx index 5ff677e587..e4529fa535 100644 --- a/frontends/ol-components/src/components/Card/ListCardCondensed.tsx +++ b/frontends/ol-components/src/components/Card/ListCardCondensed.tsx @@ -1,4 +1,10 @@ -import React, { FC, ReactNode, Children, isValidElement } from "react" +import React, { + FC, + ReactNode, + Children, + isValidElement, + AriaAttributes, +} from "react" import styled from "@emotion/styled" import { RiDraggable } from "@remixicon/react" import { theme } from "../ThemeProvider/ThemeProvider" @@ -61,10 +67,28 @@ const Content = () => <> type CardProps = { children: ReactNode[] | ReactNode className?: string + /** + * If provided, the card will render its title as a link. + * + * Clicks on the entire card can be forwarded to the link via `forwardClicksToLink`. + */ href?: string + /** + * Defaults to `false`. If `true`, clicking the whole card will click the + * href link as well. + * + * NOTES: + * - If using Card.Content to customize, you must ensure the content includes + * an anchor with the card's href. + * - Clicks will NOT be forwarded if: + * - The click target is a child of Card.Actions OR an element with + * - The click target is a child of any element with data-card-actions attribute + */ + forwardClicksToLink?: boolean draggable?: boolean onClick?: () => void -} + as?: React.ElementType +} & AriaAttributes type Card = FC & Omit @@ -74,12 +98,14 @@ const ListCardCondensed: Card = ({ href, draggable, onClick, + forwardClicksToLink = false, + ...others }) => { let content, info, title, footer, actions const hasHref = typeof href === "string" const handleHrefClick = useClickChildHref(href, onClick) - const handleClick = hasHref ? handleHrefClick : onClick + const handleClick = hasHref && forwardClicksToLink ? handleHrefClick : onClick Children.forEach(children, (child) => { if (!isValidElement(child)) return @@ -92,14 +118,14 @@ const ListCardCondensed: Card = ({ if (content) { return ( - + {content} ) } return ( - + {draggable && ( @@ -112,7 +138,7 @@ const ListCardCondensed: Card = ({
{footer}
- {actions && {actions}} + {actions && {actions}} diff --git a/frontends/ol-components/src/components/LearningResourceCard/LearningResourceCard.test.tsx b/frontends/ol-components/src/components/LearningResourceCard/LearningResourceCard.test.tsx index 02878fff8a..5187dbf505 100644 --- a/frontends/ol-components/src/components/LearningResourceCard/LearningResourceCard.test.tsx +++ b/frontends/ol-components/src/components/LearningResourceCard/LearningResourceCard.test.tsx @@ -1,5 +1,5 @@ import React from "react" -import { screen, render } from "@testing-library/react" +import { screen, render, within } from "@testing-library/react" import { LearningResourceCard } from "./LearningResourceCard" import type { LearningResourceCardProps } from "./LearningResourceCard" import { DEFAULT_RESOURCE_IMG, getReadableResourceType } from "ol-utilities" @@ -14,19 +14,29 @@ const setup = (props: LearningResourceCardProps) => { } describe("Learning Resource Card", () => { - test("Renders resource type, title and start date", () => { - const resource = factories.learningResources.resource({ - resource_type: ResourceTypeEnum.Course, - next_start_date: "2026-01-01", - }) - - setup({ resource }) - - screen.getByText("Course") - screen.getByText(resource.title) - screen.getByText("Starts:") - screen.getByText("January 01, 2026") - }) + test.each([ + { resourceType: ResourceTypeEnum.Course, expectedLabel: "Course" }, + { resourceType: ResourceTypeEnum.Program, expectedLabel: "Program" }, + ])( + "Renders resource type, title and start date as a labeled article", + ({ resourceType, expectedLabel }) => { + const resource = factories.learningResources.resource({ + resource_type: resourceType, + next_start_date: "2026-01-01", + }) + + setup({ resource }) + + const card = screen.getByRole("article", { + name: `${expectedLabel}: ${resource.title}`, + }) + + within(card).getByText(expectedLabel) + within(card).getByText(resource.title) + within(card).getByText("Starts:") + within(card).getByText("January 01, 2026") + }, + ) test("Displays run start date", () => { const resource = factories.learningResources.resource({ @@ -96,11 +106,8 @@ describe("Learning Resource Card", () => { setup({ resource, href: "/path/to/thing" }) const link = screen.getByRole("link", { - // Accessible name has resource type prepended - name: `Course: ${resource.title}`, + name: resource.title, }) - // Visual title does not have resource name prepended - expect(link.textContent).toBe(resource.title) expect(new URL(link.href).pathname).toBe("/path/to/thing") }) diff --git a/frontends/ol-components/src/components/LearningResourceCard/LearningResourceCard.tsx b/frontends/ol-components/src/components/LearningResourceCard/LearningResourceCard.tsx index ca7acb0c26..059253ad62 100644 --- a/frontends/ol-components/src/components/LearningResourceCard/LearningResourceCard.tsx +++ b/frontends/ol-components/src/components/LearningResourceCard/LearningResourceCard.tsx @@ -231,7 +231,14 @@ const LearningResourceCard: React.FC = ({ const readableType = getReadableResourceType(resource.resource_type) return ( - + = ({ - + {resource.title} diff --git a/frontends/ol-components/src/components/LearningResourceCard/LearningResourceListCard.test.tsx b/frontends/ol-components/src/components/LearningResourceCard/LearningResourceListCard.test.tsx index d3df84df46..7c635b098d 100644 --- a/frontends/ol-components/src/components/LearningResourceCard/LearningResourceListCard.test.tsx +++ b/frontends/ol-components/src/components/LearningResourceCard/LearningResourceListCard.test.tsx @@ -1,6 +1,6 @@ import React from "react" import { BrowserRouter } from "react-router-dom" -import { screen, render } from "@testing-library/react" +import { screen, render, within } from "@testing-library/react" import { LearningResourceListCard } from "./LearningResourceListCard" import type { LearningResourceListCardProps } from "./LearningResourceListCard" import { DEFAULT_RESOURCE_IMG, getReadableResourceType } from "ol-utilities" @@ -19,19 +19,29 @@ const setup = (props: LearningResourceListCardProps) => { } describe("Learning Resource List Card", () => { - test("Renders resource type, title and start date", () => { - const resource = factories.learningResources.resource({ - resource_type: ResourceTypeEnum.Course, - next_start_date: "2026-01-01", - }) + test.each([ + { resourceType: ResourceTypeEnum.Course, expectedLabel: "Course" }, + { resourceType: ResourceTypeEnum.Program, expectedLabel: "Program" }, + ])( + "Renders resource type, title and start date as a labeled article", + ({ resourceType, expectedLabel }) => { + const resource = factories.learningResources.resource({ + resource_type: resourceType, + next_start_date: "2026-01-01", + }) - setup({ resource }) + setup({ resource }) - screen.getByText("Course") - screen.getByText(resource.title) - screen.getByText("Starts:") - screen.getByText("January 01, 2026") - }) + const card = screen.getByRole("article", { + name: `${expectedLabel}: ${resource.title}`, + }) + + within(card).getByText(expectedLabel) + within(card).getByText(resource.title) + within(card).getByText("Starts:") + within(card).getByText("January 01, 2026") + }, + ) test("Displays run start date", () => { const resource = factories.learningResources.resource({ @@ -93,11 +103,8 @@ describe("Learning Resource List Card", () => { setup({ resource, href: "/path/to/thing" }) const link = screen.getByRole("link", { - // Accessible name has resource type prepended - name: `Course: ${resource.title}`, + name: resource.title, }) - // Visual title does not have resource name prepended - expect(link.textContent).toBe(resource.title) expect(link).toHaveAttribute("href", "/path/to/thing") }) diff --git a/frontends/ol-components/src/components/LearningResourceCard/LearningResourceListCard.tsx b/frontends/ol-components/src/components/LearningResourceCard/LearningResourceListCard.tsx index 9e31174bb8..1757cffa63 100644 --- a/frontends/ol-components/src/components/LearningResourceCard/LearningResourceListCard.tsx +++ b/frontends/ol-components/src/components/LearningResourceCard/LearningResourceListCard.tsx @@ -309,7 +309,14 @@ const LearningResourceListCard: React.FC = ({ } const readableType = getReadableResourceType(resource.resource_type) return ( - + = ({ - - {resource.title} - + {resource.title} {onAddToLearningPathClick && ( + @@ -134,7 +142,7 @@ const LearningResourceListCardCondensed: React.FC< {onAddToUserListClick && ( onAddToUserListClick(event, resource.id)} > {inUserList ? ( From 52fbd09d02171dea21bc0bf97451f7b199238d06 Mon Sep 17 00:00:00 2001 From: Chris Chudzicki Date: Wed, 30 Oct 2024 08:03:33 -0400 Subject: [PATCH 09/10] remove posthog console.log --- .../ConfiguredPostHogProvider/ConfiguredPostHogProvider.tsx | 4 ---- 1 file changed, 4 deletions(-) diff --git a/frontends/main/src/components/ConfiguredPostHogProvider/ConfiguredPostHogProvider.tsx b/frontends/main/src/components/ConfiguredPostHogProvider/ConfiguredPostHogProvider.tsx index 475af24453..e69b9afc5d 100644 --- a/frontends/main/src/components/ConfiguredPostHogProvider/ConfiguredPostHogProvider.tsx +++ b/frontends/main/src/components/ConfiguredPostHogProvider/ConfiguredPostHogProvider.tsx @@ -16,10 +16,6 @@ const ConfiguredPostHogProvider: React.FC<{ children: React.ReactNode }> = ({ }, } - console.log({ - postHogOptions, - }) - return apiKey ? ( {children} From 4317e8e629e8740906000dbab26bbaa1dccd6191 Mon Sep 17 00:00:00 2001 From: Chris Chudzicki Date: Wed, 30 Oct 2024 10:40:55 -0400 Subject: [PATCH 10/10] add a comment --- frontends/ol-components/src/components/Card/Card.tsx | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/frontends/ol-components/src/components/Card/Card.tsx b/frontends/ol-components/src/components/Card/Card.tsx index b69284a8d4..c4ed39316e 100644 --- a/frontends/ol-components/src/components/Card/Card.tsx +++ b/frontends/ol-components/src/components/Card/Card.tsx @@ -166,6 +166,10 @@ export const useClickChildHref = ( const target = e.target as HTMLElement if (!anchor || target.closest("a, button, [data-card-action]")) return if (e.metaKey || e.ctrlKey) { + /** + * Enables ctrl+click to open card's link in new tab. + * Without this, ctrl+click only works on the anchor itself. + */ const opts: PointerEventInit = { bubbles: false, metaKey: e.metaKey,