Skip to content

[Image] | (UX) | Show image in explore modal even when size is undefined#3367

Open
nishasy wants to merge 8 commits intomainfrom
image-no-size-explore-modal
Open

[Image] | (UX) | Show image in explore modal even when size is undefined#3367
nishasy wants to merge 8 commits intomainfrom
image-no-size-explore-modal

Conversation

@nishasy
Copy link
Contributor

@nishasy nishasy commented Mar 16, 2026

Summary:

We have a number of images in our content that are saved without a size.
If the image has a long description, it makes the "explore image" button available.
If the "explore image" button is clicked and then image has no size, the modal
shows up as completely empty.

The image should show up in the modal even if there is no size. Updating that here.

Issue: https://khanacademy.atlassian.net/browse/LEMS-3990

Test plan:

pnpm jest packages/perseus/src/widgets/image/components/explore-image-modal.test.tsx

Storybook

  • /?path=/story/widgets-image-visual-regression-tests-interactions--long-description-clicked-state-with-no-size
  • /?path=/story/widgets-image-visual-regression-tests-interactions--long-description-clicked-state-with-no-size-large-image
  • /?path=/story/widgets-image-widget-demo--large-image-with-no-size-saved
  • /?path=/story/widgets-image-widget-demo--large-image-with-no-size-saved-scale-flag
Before After
Screenshot 2026-03-16 at 4 27 55 PM Screenshot 2026-03-16 at 4 28 12 PM
Screenshot 2026-03-16 at 4 28 00 PM Screenshot 2026-03-16 at 4 28 18 PM
Before - scale feature flag after - scale feature flag
Screenshot 2026-03-16 at 4 29 58 PM Screenshot 2026-03-16 at 4 30 12 PM

@nishasy nishasy self-assigned this Mar 16, 2026
@github-actions
Copy link
Contributor

github-actions bot commented Mar 16, 2026

🗄️ Schema Change: No Changes ✅

@github-actions
Copy link
Contributor

github-actions bot commented Mar 16, 2026

Size Change: +14 B (0%)

Total Size: 486 kB

Filename Size Change
packages/perseus/dist/es/index.js 187 kB +14 B (+0.01%)
ℹ️ View Unchanged
Filename Size
packages/kas/dist/es/index.js 20.5 kB
packages/keypad-context/dist/es/index.js 1 kB
packages/kmath/dist/es/index.js 5.96 kB
packages/math-input/dist/es/index.js 98.5 kB
packages/math-input/dist/es/strings.js 1.61 kB
packages/perseus-core/dist/es/index.item-splitting.js 11.8 kB
packages/perseus-core/dist/es/index.js 24.9 kB
packages/perseus-editor/dist/es/index.js 100 kB
packages/perseus-linter/dist/es/index.js 8.83 kB
packages/perseus-score/dist/es/index.js 9.4 kB
packages/perseus-utils/dist/es/index.js 403 B
packages/perseus/dist/es/strings.js 7.47 kB
packages/pure-markdown/dist/es/index.js 1.39 kB
packages/simple-markdown/dist/es/index.js 6.71 kB

compressed-size-action

@github-actions
Copy link
Contributor

github-actions bot commented Mar 16, 2026

🛠️ Item Splitting: No Changes ✅

@github-actions
Copy link
Contributor

github-actions bot commented Mar 16, 2026

npm Snapshot: Published

Good news!! We've packaged up the latest commit from this PR (82c5b8b) and published it to npm. You
can install it using the tag PR3367.

Example:

pnpm add @khanacademy/perseus@PR3367

If you are working in Khan Academy's frontend, you can run the below command.

./dev/tools/bump_perseus_version.ts -t PR3367

If you are working in Khan Academy's webapp, you can run the below command.

./dev/tools/bump_perseus_version.js -t PR3367

// bgWidth / bgHeight = X / modalImageHeight
// => X = (bgWidth / bgHeight) * modalImageHeight
width =
(backgroundImage.width / backgroundImage.height) * modalImageHeight;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I considered removing this block entirely, since the sizing just works with undefined now. However, I opted to keep this block here since it will be useful for the work in my other branch on scaled graphie labels.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file has a lot of changes because of the boilerplate code that had to be updated to allow the image to actually load for testing.

@nishasy nishasy requested review from a team and mark-fitzgerald March 16, 2026 23:31
!backgroundImage.height ||
!backgroundImage.width ||
!backgroundImage.url
) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The culprit

background-color: var(--wb-semanticColor-core-background-base-default);
/* Stop large images from overflowing out of the image area of the modal. */
max-width: 100%;
max-height: var(--modal-panel-height);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Large images would overflow and cover the long description without this CSS.

Copy link
Contributor

Choose a reason for hiding this comment

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

This CSS looks appropriate. However, I would prefer some changes to how the CSS variables are handled. I realize that this may be beyond the intent of this PR, but these changes will help to avoid potential conflicts.

  1. Namespace the CSS variables
    • Instead of --modal-panel-height, I recommend using --perseus-image-modal-panel-height
    • This will prevent naming conflicts with other parts of the application that have a modal panel height variable of the same name.
  2. Attach the CSS variables to .modal-container instead of :root
    • Even if variables are namespaced, adding them to the :root element overloads that element with variables that don't apply anywhere else other than the image widget.
    • If the variables are NOT namespaced, then this runs the risk of overriding the variable if it exists anywhere else in frontend, or of being overridden by a setting elsewhere in frontend.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you!

@nishasy nishasy marked this pull request as ready for review March 16, 2026 23:32
@claude
Copy link

claude bot commented Mar 16, 2026

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review to trigger a review.

@nishasy
Copy link
Contributor Author

nishasy commented Mar 16, 2026

Tested it in my local server with a PR tag using the exercise that I first noticed this issue in:

Before After
Screenshot 2026-03-16 at 4 40 42 PM Screenshot 2026-03-16 at 4 40 36 PM

display: flex;
width: 100%;
justify-content: center;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mark-fitzgerald Is it okay for .modal-container to be defined in two spots like this to separate out where the variables are defined?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants