-
Notifications
You must be signed in to change notification settings - Fork 591
fix: artist bio rendering on Artwork screen #13842
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
Changes from all commits
e82d447
9949616
021440f
6af6ea2
19c9b9e
de8f588
239df27
068cd13
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| import { | ||
| Color, | ||
| Flex, | ||
| FlexProps, | ||
| TextProps, | ||
|
|
@@ -15,6 +16,7 @@ import RenderHtml, { CustomBlockRenderer, MixedStyleRecord } from "react-native- | |
|
|
||
| interface HTMLProps extends FlexProps { | ||
| html: string | ||
| color?: Color | ||
| onLinkPress?: (href: string) => void | ||
| tagStyles?: MixedStyleRecord | ||
| variant?: TextProps["variant"] | ||
|
|
@@ -29,6 +31,7 @@ export const FONTS = { | |
|
|
||
| export const HTML: React.FC<HTMLProps> = ({ | ||
| html, | ||
| color: textColor = "mono100", | ||
| onLinkPress, | ||
| tagStyles = {}, | ||
| variant = "sm", | ||
|
|
@@ -47,6 +50,7 @@ export const HTML: React.FC<HTMLProps> = ({ | |
| <RenderHtml | ||
| contentWidth={contentWidth} | ||
| source={{ html }} | ||
| baseStyle={{ color: color(textColor) }} | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Adding |
||
| systemFonts={[FONTS.regular, FONTS.italic, FONTS.medium, FONTS.mediumItalic]} | ||
| renderers={{ | ||
| h2: CustomH2Renderer as CustomBlockRenderer, | ||
|
|
@@ -71,25 +75,25 @@ export const HTML: React.FC<HTMLProps> = ({ | |
| { | ||
| a: { | ||
| textDecorationLine: "underline", | ||
| textDecorationColor: color("mono100"), | ||
| color: color("mono100"), | ||
| textDecorationColor: color(textColor), | ||
| color: color(textColor), | ||
|
github-actions[bot] marked this conversation as resolved.
|
||
| textDecorationStyle: "solid", | ||
| }, | ||
| p: { | ||
| fontFamily: FONTS.regular, | ||
| color: color("mono100"), | ||
| color: color(textColor), | ||
| marginBottom: "1em", | ||
| ...variantStyles, | ||
| }, | ||
| li: { | ||
| fontFamily: FONTS.regular, | ||
| color: color("mono100"), | ||
| color: color(textColor), | ||
| marginBottom: "1em", | ||
| ...omit(variantStyles, "lineHeight"), // to prevent mis-aligned markers | ||
| }, | ||
| em: { | ||
| fontFamily: FONTS.italic, | ||
| color: color("mono100"), | ||
| color: color(textColor), | ||
| }, | ||
| h1: { | ||
| fontFamily: FONTS.medium, | ||
|
|
@@ -98,7 +102,7 @@ export const HTML: React.FC<HTMLProps> = ({ | |
| letterSpacing: theme.textTreatments["xl"].letterSpacing, | ||
| marginBottom: "1em", | ||
| fontWeight: "normal", | ||
| color: color("mono100"), | ||
| color: color(textColor), | ||
| }, | ||
| h2: { | ||
| fontFamily: FONTS.medium, | ||
|
|
@@ -107,7 +111,7 @@ export const HTML: React.FC<HTMLProps> = ({ | |
| letterSpacing: theme.textTreatments["lg-display"].letterSpacing, | ||
| marginBottom: "1em", | ||
| fontWeight: "normal", | ||
| color: color("mono100"), | ||
| color: color(textColor), | ||
| }, | ||
| h3: { | ||
| fontFamily: FONTS.regular, | ||
|
|
@@ -116,7 +120,7 @@ export const HTML: React.FC<HTMLProps> = ({ | |
| letterSpacing: theme.textTreatments["md"].letterSpacing, | ||
| marginBottom: "1em", | ||
| fontWeight: "normal", | ||
| color: color("mono100"), | ||
| color: color(textColor), | ||
| }, | ||
| }, | ||
| tagStyles | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| import { screen } from "@testing-library/react-native" | ||
| import { HTML } from "app/Components/HTML" | ||
| import { renderWithWrappers } from "app/utils/tests/renderWithWrappers" | ||
|
|
||
| describe("HTML", () => { | ||
| it("applies the color prop to bare text with no wrapping tag", () => { | ||
| renderWithWrappers(<HTML html="Bare text with no tag" color="mono0" />) | ||
|
|
||
| const node = screen.getByText("Bare text with no tag") | ||
| expect(node).toHaveStyle({ color: "#FFFFFF" }) | ||
| }) | ||
|
|
||
| it("applies the color prop to tags without an explicit tagsStyles entry", () => { | ||
| renderWithWrappers(<HTML html="<strong>Bold text</strong>" color="mono0" />) | ||
|
|
||
| const node = screen.getByText("Bold text") | ||
| expect(node).toHaveStyle({ color: "#FFFFFF" }) | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,16 +1,22 @@ | ||||||||||
| import { Spacer, Flex, Box, Text, Join, Screen } from "@artsy/palette-mobile" | ||||||||||
| import { Spacer, Flex, Box, Text, LinkText, Join, Screen } from "@artsy/palette-mobile" | ||||||||||
| import { AboutArtist_artwork$data } from "__generated__/AboutArtist_artwork.graphql" | ||||||||||
| import { ArtistListItemContainer as ArtistListItem } from "app/Components/ArtistListItem" | ||||||||||
| import { ReadMore } from "app/Components/ReadMore" | ||||||||||
| import { HTML } from "app/Components/HTML" | ||||||||||
| import { truncatedTextLimit } from "app/utils/hardware" | ||||||||||
| import { Schema } from "app/utils/track" | ||||||||||
| import { truncateHtml } from "app/utils/truncateHtml" | ||||||||||
| import { useState } from "react" | ||||||||||
| import { createFragmentContainer, graphql } from "react-relay" | ||||||||||
| import { useTracking } from "react-tracking" | ||||||||||
|
|
||||||||||
| interface AboutArtistProps { | ||||||||||
| artwork: AboutArtist_artwork$data | ||||||||||
| } | ||||||||||
|
|
||||||||||
| export const AboutArtist: React.FC<AboutArtistProps> = ({ artwork }) => { | ||||||||||
| const tracking = useTracking() | ||||||||||
| const [expanded, setExpanded] = useState(false) | ||||||||||
|
|
||||||||||
| const artists = artwork.artists || [] | ||||||||||
|
|
||||||||||
| const hasSingleArtist = artists && artists.length === 1 | ||||||||||
|
|
@@ -29,6 +35,22 @@ export const AboutArtist: React.FC<AboutArtistProps> = ({ artwork }) => { | |||||||||
| const backgroundColor = artwork.isUnlisted ? "mono100" : "mono0" | ||||||||||
| const textColor = artwork.isUnlisted ? "mono0" : "mono100" | ||||||||||
|
|
||||||||||
| const { text: truncatedText, wasTruncated: canExpand } = text | ||||||||||
| ? truncateHtml(text, textLimit) | ||||||||||
| : { text: undefined, wasTruncated: false } | ||||||||||
|
Comment on lines
+38
to
+40
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The ternary only exists to avoid calling
Suggested change
|
||||||||||
|
|
||||||||||
| const handleExpandPress = () => { | ||||||||||
| if (!expanded) { | ||||||||||
| tracking.trackEvent({ | ||||||||||
| action_name: Schema.ActionNames.ReadMore, | ||||||||||
| action_type: Schema.ActionTypes.Tap, | ||||||||||
| context_module: Schema.ContextModules.ArtistBiography, | ||||||||||
| flow: Schema.Flow.AboutTheArtist, | ||||||||||
| }) | ||||||||||
| } | ||||||||||
| setExpanded((prev) => !prev) | ||||||||||
| } | ||||||||||
|
|
||||||||||
| return ( | ||||||||||
| <Screen.FullWidthItem p={2} backgroundColor={backgroundColor}> | ||||||||||
| <Flex alignItems="flex-start"> | ||||||||||
|
|
@@ -51,16 +73,21 @@ export const AboutArtist: React.FC<AboutArtistProps> = ({ artwork }) => { | |||||||||
| </Flex> | ||||||||||
| {!!hasSingleArtist && !!text && !!artwork.displayArtistBio && ( | ||||||||||
| <Box mt={2} mb={artwork.isUnlisted ? 1 : 0}> | ||||||||||
| <ReadMore | ||||||||||
| content={text} | ||||||||||
| contextModule={Schema.ContextModules.ArtistBiography} | ||||||||||
| maxChars={textLimit} | ||||||||||
| textStyle="new" | ||||||||||
| trackingFlow={Schema.Flow.AboutTheArtist} | ||||||||||
| textVariant="sm" | ||||||||||
| linkTextVariant="sm-display" | ||||||||||
| <HTML | ||||||||||
| html={expanded ? text : `${truncatedText}${canExpand ? "... " : ""}`} | ||||||||||
| color={textColor} | ||||||||||
| variant="sm" | ||||||||||
| /> | ||||||||||
| {!!canExpand && ( | ||||||||||
| <LinkText | ||||||||||
| variant="sm-display" | ||||||||||
| color={textColor} | ||||||||||
| accessibilityRole="button" | ||||||||||
| onPress={handleExpandPress} | ||||||||||
| > | ||||||||||
| {expanded ? "Read Less" : "Read More"} | ||||||||||
| </LinkText> | ||||||||||
| )} | ||||||||||
|
anandaroop marked this conversation as resolved.
anandaroop marked this conversation as resolved.
|
||||||||||
| </Box> | ||||||||||
| )} | ||||||||||
| </Screen.FullWidthItem> | ||||||||||
|
|
@@ -73,7 +100,7 @@ export const AboutArtistFragmentContainer = createFragmentContainer(AboutArtist, | |||||||||
| displayArtistBio | ||||||||||
| artists(shallow: true) { | ||||||||||
| id | ||||||||||
| biographyBlurb(partnerBio: false) { | ||||||||||
| biographyBlurb(format: HTML, partnerBio: false) { | ||||||||||
| text | ||||||||||
| } | ||||||||||
|
|
||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -85,7 +85,7 @@ describe("AboutArtist", () => { | |
| }), | ||
| }) | ||
|
|
||
| const readMoreLink = screen.getByText("Read more") | ||
| const readMoreLink = screen.getByText("Read More") | ||
| fireEvent.press(readMoreLink) | ||
| expect(mockTrackEvent).toBeCalledWith({ | ||
| action_name: "readMore", | ||
|
|
@@ -94,4 +94,26 @@ describe("AboutArtist", () => { | |
| flow: "AboutTheArtist", | ||
| }) | ||
| }) | ||
|
|
||
| it("renders HTML-formatted bio content without raw markdown markers", () => { | ||
| renderWithRelay({ | ||
| Artwork: () => ({ | ||
| isUnlisted: false, | ||
| displayArtistBio: true, | ||
| artists: [ | ||
| { | ||
| biographyBlurb: { | ||
| text: "<h3>Key Exhibitions</h3><ul><li>David Hockney, The Metropolitan Museum of Art, 2017</li></ul>", | ||
| }, | ||
| }, | ||
| ], | ||
| }), | ||
| }) | ||
|
|
||
| expect(screen.getByText("Key Exhibitions")).toBeOnTheScreen() | ||
| expect( | ||
| screen.getByText("David Hockney, The Metropolitan Museum of Art, 2017") | ||
| ).toBeOnTheScreen() | ||
| expect(screen.queryByText(/<h3>/)).not.toBeOnTheScreen() | ||
| }) | ||
|
Comment on lines
+98
to
+118
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The unlisted/inverted variant is the reason the
Also missing: expand/collapse for this component. |
||
| }) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,80 @@ | ||
| import { truncateHtml } from "app/utils/truncateHtml" | ||
|
|
||
| describe("truncateHtml", () => { | ||
| it("behaves like a plain slice when there is no markup", () => { | ||
| const { text, wasTruncated } = truncateHtml("Hello world", 5) | ||
|
|
||
| expect(text).toEqual("Hello") | ||
| expect(wasTruncated).toBe(true) | ||
| }) | ||
|
|
||
| it("returns the entire string, untruncated, when the budget exceeds the visible text length", () => { | ||
| const html = "<p>Hi</p>" | ||
| const { text, wasTruncated } = truncateHtml(html, 100) | ||
|
|
||
| expect(text).toEqual(html) | ||
| expect(wasTruncated).toBe(false) | ||
| }) | ||
|
|
||
| it("does not count tag markup toward the visible character budget", () => { | ||
| const html = "<p>Hello</p>" | ||
| const { text, wasTruncated } = truncateHtml(html, 5) | ||
|
|
||
| expect(text).toEqual("<p>Hello</p>") | ||
| expect(wasTruncated).toBe(false) | ||
| }) | ||
|
|
||
| it("stops as soon as the visible character budget is reached, dropping trailing markup", () => { | ||
| const html = "<p>Hello world</p>" | ||
| const { text, wasTruncated } = truncateHtml(html, 5) | ||
|
|
||
| expect(text).toEqual("<p>Hello") | ||
| expect(wasTruncated).toBe(true) | ||
| }) | ||
|
|
||
| it("never cuts in the middle of a tag or attribute", () => { | ||
| const html = | ||
| '<a href="https://www.artsy.net/artist/david-hockney">Hockney</a> retrospective' | ||
|
|
||
| const { text, wasTruncated } = truncateHtml(html, 3) | ||
|
|
||
| expect(text).toEqual('<a href="https://www.artsy.net/artist/david-hockney">Hoc') | ||
| expect(wasTruncated).toBe(true) | ||
| }) | ||
|
|
||
| it("does not cut in the middle of an entity reference", () => { | ||
| const { text } = truncateHtml("Arts & Crafts", 7) | ||
|
|
||
| expect(text).toEqual("Arts & ") | ||
| }) | ||
|
|
||
| it("treats a bare ampersand that is not part of an entity as a normal character", () => { | ||
| const { text } = truncateHtml("Fish & Chips", 6) | ||
|
|
||
| expect(text).toEqual("Fish &") | ||
| }) | ||
|
|
||
| it("stops before a new tag opens once the budget is spent, without leaving it dangling open", () => { | ||
| const html = "<p>abc</p><h3>Key Exhibitions</h3>" | ||
| const { text } = truncateHtml(html, 3) | ||
|
|
||
| expect(text).toEqual("<p>abc</p>") | ||
| }) | ||
|
|
||
| it("still emits closing tags for elements opened before the budget was spent", () => { | ||
| const html = "<p>abc</p>" | ||
| const { text } = truncateHtml(html, 3) | ||
|
|
||
| expect(text).toEqual(html) | ||
| }) | ||
|
|
||
| it("keeps the returned text and wasTruncated flag consistent even on malformed HTML", () => { | ||
| // An unterminated "<b" tag has no single answer for "how much is visible", but the | ||
| // scanner's own text and wasTruncated always agree with each other by construction, | ||
| // since both come from the same pass rather than two independently computed answers. | ||
| const { text, wasTruncated } = truncateHtml("Hi <b", 5) | ||
|
|
||
| expect(text).toEqual("Hi <b") | ||
| expect(wasTruncated).toBe(false) | ||
| }) | ||
| }) |
Uh oh!
There was an error while loading. Please reload this page.