Expandable images in lesson plans #38673

Hamms · 2021-01-21T22:30:42Z

Add support for our existing "expandable images" functionality in lesson plans.

Specifically:

Extract the "after markdown is rendered, query the resulting HTML and look for expandable images" code to a reusable place
Create a reusable EnhancedSafeMarkdown component which uses the existing SafeMarkdown component as well as the expandable images code. (This component will also be a good place in the future to extract other things we want to do after rendering markdown, like Blockly blocks)
Extract the styles used to display the clickable image thumbnail to a reusable place, then add some Lesson-plan-specific CSS and reference those styles
Add a basic React component for rendering the expanded image outside the context of Code Studio. I decided to add a new component for this rather than extracting the existing functionality because the existing functionality is super tangled up in a bunch of code studio-specific stuff. I did try to use the same hooks where possible (ie, the same redux reducer).

Screenshots


before
after
after (expanded)

Testing story

Added some basic unit tests. I'd also like to add a UI test, but I couldn't figure out a good way to do that given that we don't yet have a url scheme for lesson plans that's consistent across environments. Added a jira item to track doing this as follow-up work once we have those urls

Follow-up work

There's quite a bit of tech debt around expandable images, and around our various dialog components in general. In particular, the original implementation of expandable images was based on the dialog component we used to use for instructions, which is now much less ubiquitous than it used to be.

There's also probably a significant extent to which the styles of both the clickable image thumbnail and the expanded image dialog could be improved.

Links

Reviewer Checklist:

Tests provide adequate coverage
Privacy and Security impacts have been assessed
Code is well-commented
New features are translatable or updates will not break translations
Relevant documentation has been added or updated
User impact is well-understood and desirable
Pull Request is labeled appropriately
Follow-up work items (including potential tech debt) are tracked and linked

…mponent, so we can reuse it elsewhere

…ering needs

…ent to a common location, then add a new 'lessons.scss' for lesson plan styles and have it reference the aniGif styles

…lans

Hamms · 2021-01-22T21:20:34Z

One thing to note: right now, the expanded image dialog is only enabled for the "show" view, not the "edit" view; this means that although the clickable image thumbnail will display correctly in the editor preview, clicking the image won't actually do anything. Does anyone think that functionality is worth adding?

Specifically, rework them to use a progressive enhancement model where the component wraps its content in individual components for each enhancement. The specific motivation here is to isolate the "needs to be connected to redux" requirement to just the case when expandable images is actually enabled.

davidsbailey

Great code comments! I've got a question inline.

davidsbailey · 2021-01-25T22:36:24Z

apps/test/unit/templates/EnhancedSafeMarkdownTest.js

+
+import * as expandableImages from '@cdo/apps/templates/utils/expandableImages';
+import {StatelessEnhancedSafeMarkdown as EnhancedSafeMarkdown} from '@cdo/apps/templates/EnhancedSafeMarkdown';
+import {expect} from '../../util/deprecatedChai';


please import reconfiguredChai instead

davidsbailey · 2021-01-25T22:36:49Z

apps/test/unit/templates/utils/expandableImagesTest.js

+import ReactDOM from 'react-dom';
+import sinon from 'sinon';
+
+import {expect} from '../../../util/deprecatedChai';


please import reconfiguredChai instead

davidsbailey · 2021-01-25T22:45:05Z

apps/src/templates/utils/expandableImages.js

+     * > instead of calling ReactDOM.render.
+     *
+     * We should probably rebuild this in such a way as to not violate React's
+     * expectations like this.


I think I am missing something here. the way this is implemented in this PR, do we always hit this warning? If not, what are the criteria for calling this function without triggering this warning?

Yes, we always hit this warning. Note that this PR is not implementing this functionality, it's just reorganizing it (specifically, extracting it from the MarkdownInstructions component). We currently always hit this warning when rendering expandable images in the context of React. (For historical context, expandable images were originally implemented outside the context of React)

In the interest of aligning to the "do no harm" approach to tech debt we've settled on, I decided not to fix the implementation as part of this work but instead to add this comment. Although, we should probably also add a work item somewhere to try to get that work on an actual schedule somewhere. Any suggestions for where/when?

Thanks Elijah, all makes sense. This could go into the Q2 Tech Debt sprint in Jira.

davidsbailey · 2021-01-25T22:58:02Z

apps/style/common.scss

-    top: 0px;
-  }
-}
+@import "aniGif";


Nice extraction! I would have expected "common.css" to be included "everywhere" in dashboard, but it appears to only be "common" to all of our "studio app" level pages (applab, artist, etc).

davidsbailey

Looks great, @Hamms ! Seems like a great balance of cleaning up a bit now, and putting off fixing of existing bugs until after our deadline.

dmcavoy · 2021-01-26T17:54:15Z

One thing to note: right now, the expanded image dialog is only enabled for the "show" view, not the "edit" view; this means that although the clickable image thumbnail will display correctly in the editor preview, clicking the image won't actually do anything. Does anyone think that functionality is worth adding?

I think this should be the goal we would like to work too. If it is a bunch of work to make happen maybe we consider it as something we do in Q2?

dmcavoy · 2021-01-26T17:56:05Z

Nit: Should there be a button at the bottom of the pop up dialog that you can click to close it?

dmcavoy · 2021-01-26T17:59:07Z

apps/src/templates/EnhancedSafeMarkdown.jsx

+import {openDialog} from '@cdo/apps/redux/instructionsDialog';
+import {renderExpandableImages} from './utils/expandableImages';
+
+export class UnconnectedExpandableImagesWrapper extends React.Component {


I don't know how others feel but I general find it easier to navigate the code base when react components each have their own file instead of multiple in one file.

The idea here is that this is not a independent component which code outside of this file should reference or even know about; it's only being exported to facilitate testing. If in the future we do decide that we want to use this code outside the context of the EnhancedSafeMarkdown component, we can put it in its own file then.

dmcavoy · 2021-01-26T17:59:50Z

apps/src/templates/EnhancedSafeMarkdown.jsx

+  }
+
+  componentDidUpdate(prevProps) {
+    // TODO: do we need to do any kind of cleanup here? Or otherwise do


I'm not sure if I hit this TODO I would know what to do with it

I didn't either, that's why I left it as a todo rather than just doing the work myself. TODOs aren't intended to be calls to action, they're intended to be callouts to future engineers who find themselves in this part of the codebase. "If you're looking for something that could use some improvement, or hunting down a bug and are not sure where it might be, here's some code that could use attention"

Hamms · 2021-01-26T20:35:20Z

Nit: Should there be a button at the bottom of the pop up dialog that you can click to close it?

Debatably, but the goal here is to enable an existing feature on a new platform; we can discuss potential improvements to the feature as separate work items.

…re now using EnhancedSafeMarkdown

Hamms added 5 commits January 21, 2021 14:29

extract the 'expandable images' code from the MarkdownInstructions co…

b7a2f3b

…mponent, so we can reuse it elsewhere

add some jsdoc comments and TODOs

f509fa4

add an EnhancedSafeMarkdown component for all our extra-markdown rend…

2ecc784

…ering needs

extract 'aniGif' styles which are used by the expandable image compon…

3982984

…ent to a common location, then add a new 'lessons.scss' for lesson plan styles and have it reference the aniGif styles

add a basic React Dialog for displaying expandable images in lesson p…

4bc7fc9

…lans

add some basic unit tests for new expandable images functionality

5c3d079

Hamms marked this pull request as ready for review January 23, 2021 02:15

Hamms requested review from davidsbailey, dmcavoy and bethanyaconnor January 23, 2021 02:15

Hamms added 2 commits January 25, 2021 12:16

Merge branch 'staging' into expandable-images-in-lesson-plans

6839491

davidsbailey reviewed Jan 25, 2021

View reviewed changes

use currently-supported rather than deprecated version of chai

3787d95

davidsbailey approved these changes Jan 26, 2021

View reviewed changes

bethanyaconnor approved these changes Jan 26, 2021

View reviewed changes

dmcavoy reviewed Jan 26, 2021

View reviewed changes

Hamms added 2 commits January 26, 2021 14:21

fix a misbehaving import statement

0de7b2b

update tests for Components which used to be using SafeMarkdown but a…

7eba630

…re now using EnhancedSafeMarkdown

Hamms merged commit 4dc8efd into staging Jan 27, 2021

Hamms deleted the expandable-images-in-lesson-plans branch January 27, 2021 22:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expandable images in lesson plans #38673

Expandable images in lesson plans #38673

Hamms commented Jan 21, 2021 •

edited

Hamms commented Jan 22, 2021

davidsbailey left a comment

davidsbailey Jan 25, 2021

davidsbailey Jan 25, 2021

davidsbailey Jan 25, 2021

Hamms Jan 25, 2021 •

edited

davidsbailey Jan 25, 2021

davidsbailey Jan 25, 2021

davidsbailey left a comment

dmcavoy commented Jan 26, 2021

dmcavoy commented Jan 26, 2021

dmcavoy Jan 26, 2021

Hamms Jan 26, 2021

dmcavoy Jan 26, 2021

Hamms Jan 26, 2021

Hamms commented Jan 26, 2021

Expandable images in lesson plans #38673

Expandable images in lesson plans #38673

Conversation

Hamms commented Jan 21, 2021 • edited

Screenshots

Testing story

Follow-up work

Links

Reviewer Checklist:

Hamms commented Jan 22, 2021

davidsbailey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hamms Jan 25, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidsbailey left a comment

Choose a reason for hiding this comment

dmcavoy commented Jan 26, 2021

dmcavoy commented Jan 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hamms commented Jan 26, 2021

Hamms commented Jan 21, 2021 •

edited

Hamms Jan 25, 2021 •

edited