Skip to content

Commit

Permalink
Wrap SvgImage in AssetContext's (#816)
Browse files Browse the repository at this point in the history
## Summary:

While trying to figure out why some Cypress tests weren't showing a background image on graphs, I discovered that we don't wrap all of our `<SvgImage />` components in an `AssetContext`. 

The `AssetContext` is used to communicate back up to the host application when things that happen asynchronously are completed. This allows the host to wait until all of the asynchronous assets are loaded/rendered (such as loading images or rendering Math). 

Wrapping these instances seems to fix the Cypress tests not showing background images. 

Issue: "none"

## Test plan:

Author: jeremywiebe

Reviewers: jeremywiebe, benchristel, SonicScrewdriver, handeyeco, nedredmond

Required Reviewers:

Approved By: benchristel

Checks: ✅ codecov/project, ✅ codecov/patch, ✅ Upload Coverage, ✅ Publish npm snapshot (ubuntu-latest, 16.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 16.x), ✅ Cypress (ubuntu-latest, 16.x), ✅ Extract i18n strings (ubuntu-latest, 16.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 16.x), ✅ Jest Coverage (ubuntu-latest, 16.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 16.x), ✅ Check builds for changes in size (ubuntu-latest, 16.x), ✅ gerald

Pull Request URL: #816
  • Loading branch information
jeremywiebe authored Nov 28, 2023
1 parent 971bf6c commit cdcd162
Show file tree
Hide file tree
Showing 8 changed files with 81 additions and 42 deletions.
5 changes: 5 additions & 0 deletions .changeset/calm-dryers-itch.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/perseus": minor
---

Wrap all SvgImage instances in AssetContext (ensures that host applications can correctly wait for all images to load in a renderer before proceeding).
2 changes: 1 addition & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,4 @@ coverage-ts
storybook-static
.nyc_output
*.tsbuildinfo
cypress/
/cypress/
7 changes: 6 additions & 1 deletion config/cypress/cypress.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,16 @@ const resourceTypes = /\.(png|svg|eot|woff|woff2|ttf|otf|svg(#Symbola)?)$/;
module.exports = defineConfig({
fixturesFolder: false,
video: false,

// Prevent Cypress from scrolling to elements before clicking them.
scrollBehavior: false,
// iPhone 14/15 Pro Max
viewportWidth: 430,
viewportHeight: 932,
component: {
specPattern: ["packages/**/*.cypress.{js,ts,jsx,tsx}"],
indexHtmlFile: "config/cypress/component-index.html",
supportFile: "config/cypress/support.js",

devServer: {
bundler: "webpack",
framework: "react",
Expand Down
16 changes: 15 additions & 1 deletion config/cypress/support.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,20 @@ if (Cypress.env("CYPRESS_COVERAGE")) {
// NOTE: If we end up with a lot of custom commands, we should break
// each command into its own file.

// TODO(LC-1495): Leaving this here for the future where we migrate this to TS.
// It works, but switching our Cypress config to .ts files causes Cypress types
// to conflict with Jest types. The following URL looks like it would fix it,
// but I couldn't get it working in a short timebox. Leaving this breadcrumb
// trail for future!
// https://docs.cypress.io/guides/tooling/typescript-support#Clashing-Types-with-Jest
// declare global {
// namespace Cypress {
// interface Chainable {
// dragTo(position: {x: number; y: number}): Chainable<any>;
// }
// }
// }

/**
* Click a node and drag it to the specified {x, y} position
*/
Expand All @@ -22,5 +36,5 @@ const dragTo = (node, pos) => {
.trigger("mouseup", {force: true})
.trigger("mouseout", {force: true});
};
// @ts-expect-error - TS2769 - No overload matches this call.

Cypress.Commands.add("dragTo", {prevSubject: true}, dragTo);
File renamed without changes.
22 changes: 14 additions & 8 deletions packages/perseus/src/components/graph.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import * as React from "react";
import ReactDOM from "react-dom";
import _ from "underscore";

import AssetContext from "../asset-context";
import {interactiveSizes} from "../styles/constants";
import Util from "../util";
import GraphUtils from "../util/graph-utils";
Expand Down Expand Up @@ -395,14 +396,19 @@ class Graph extends React.Component<Props> {
if (imageData.url) {
const scale = this.props.box[0] / interactiveSizes.defaultBoxSize;
image = (
// @ts-expect-error - TS2741 - Property 'alt' is missing in type '{ src: any; width: any; height: any; scale: number; responsive: false; }' but required in type 'Pick<Readonly<Props> & Readonly<{ children?: ReactNode; }>, "children" | "height" | "width" | "title" | "alt" | "trackInteraction" | "preloader" | "allowFullBleed" | "extraGraphie" | "overrideAriaHidden">'.
<SvgImage
src={imageData.url}
width={imageData.width}
height={imageData.height}
scale={scale}
responsive={false}
/>
<AssetContext.Consumer>
{({setAssetStatus}) => (
<SvgImage
// @ts-expect-error - TS2741 - Property 'alt' is missing in type '{ src: any; width: any; height: any; scale: number; responsive: false; }' but required in type 'Pick<Readonly<Props> & Readonly<{ children?: ReactNode; }>, "children" | "height" | "width" | "title" | "alt" | "trackInteraction" | "preloader" | "allowFullBleed" | "extraGraphie" | "overrideAriaHidden">'.
src={imageData.url}
width={imageData.width}
height={imageData.height}
scale={scale}
responsive={false}
setAssetStatus={setAssetStatus}
/>
)}
</AssetContext.Consumer>
);
} else {
image = null;
Expand Down
24 changes: 13 additions & 11 deletions packages/perseus/src/components/svg-image.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/* eslint-disable @khanacademy/ts-no-error-suppressions */
/* eslint-disable no-useless-escape, react/no-unsafe */
/* eslint-disable react/no-unsafe */
import {CircularSpinner} from "@khanacademy/wonder-blocks-progress-spinner";
import classNames from "classnames";
import $ from "jquery";
Expand Down Expand Up @@ -79,7 +79,7 @@ const doJSONP = function (url: string, options) {
// system (with file://). We replace urls that start with `web+graphie`
// in the perseus json with this `file+graphie` prefix to indicate that
// they should have the `file://` protocol instead of `https://`.
const svgLocalLabelsRegex = /^file\+graphie\:/;
const svgLocalLabelsRegex = /^file\+graphie:/;
const hashRegex = /\/([^/]+)$/;

function isImageProbablyPhotograph(imageUrl) {
Expand Down Expand Up @@ -180,6 +180,16 @@ type Props = {
setAssetStatus: (assetKey: string, loaded: boolean) => void;
};

type DefaultProps = {
constrainHeight: NonNullable<Props["constrainHeight"]>;
onUpdate: NonNullable<Props["onUpdate"]>;
responsive: NonNullable<Props["responsive"]>;
scale: NonNullable<Props["scale"]>;
setAssetStatus: NonNullable<Props["setAssetStatus"]>;
src: NonNullable<Props["src"]>;
zoomToFullSizeOnMobile: NonNullable<Props["zoomToFullSizeOnMobile"]>;
};

type Label = {
coordinates: ReadonlyArray<any>;
content: string;
Expand Down Expand Up @@ -212,15 +222,7 @@ type State = {
class SvgImage extends React.Component<Props, State> {
_isMounted: boolean;

static defaultProps: {
constrainHeight: boolean;
onUpdate: () => void;
responsive: boolean;
scale: number;
setAssetStatus: (src: string, status: boolean) => void;
src: string;
zoomToFullSizeOnMobile: boolean;
} = {
static defaultProps: DefaultProps = {
constrainHeight: false,
onUpdate: () => {},
responsive: true,
Expand Down
47 changes: 27 additions & 20 deletions packages/perseus/src/widgets/image.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import classNames from "classnames";
import * as React from "react";
import _ from "underscore";

import AssetContext from "../asset-context";
import SvgImage from "../components/svg-image";
import * as Changeable from "../mixins/changeable";
import Renderer from "../renderer";
Expand Down Expand Up @@ -93,11 +94,14 @@ class ImageWidget extends React.Component<Props> {
const backgroundImage = this.props.backgroundImage;

if (backgroundImage.url) {
const url = backgroundImage.url;
image = (
<SvgImage
src={backgroundImage.url}
alt={
/* alt text is formatted in a sr-only
<AssetContext.Consumer>
{({setAssetStatus}) => (
<SvgImage
src={url}
alt={
/* alt text is formatted in a sr-only
div next to the image in addition to
the alt attribute.
If there is no alt text at all,
Expand All @@ -117,22 +121,25 @@ class ImageWidget extends React.Component<Props> {
in practice right now, although
it will exhibit weird behaviour
while editing. */
this.props.alt
}
overrideAriaHidden={true}
width={backgroundImage.width}
height={backgroundImage.height}
preloader={apiOptions.imagePreloader}
extraGraphie={{
box: this.props.box,
range: this.props.range,
labels: this.props.labels,
}}
trackInteraction={this.props.trackInteraction}
zoomToFullSizeOnMobile={apiOptions.isMobile}
constrainHeight={apiOptions.isMobile}
allowFullBleed={apiOptions.isMobile}
/>
this.props.alt
}
overrideAriaHidden={true}
width={backgroundImage.width}
height={backgroundImage.height}
preloader={apiOptions.imagePreloader}
extraGraphie={{
box: this.props.box,
range: this.props.range,
labels: this.props.labels,
}}
trackInteraction={this.props.trackInteraction}
zoomToFullSizeOnMobile={apiOptions.isMobile}
constrainHeight={apiOptions.isMobile}
allowFullBleed={apiOptions.isMobile}
setAssetStatus={setAssetStatus}
/>
)}
</AssetContext.Consumer>
);
}

Expand Down

0 comments on commit cdcd162

Please sign in to comment.