Skip to content
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

Improvements to editing expandable images #39336

Merged
merged 6 commits into from
Mar 5, 2021
Merged

Conversation

dmcavoy
Copy link
Contributor

@dmcavoy dmcavoy commented Mar 4, 2021

Adds a checkbox which allows an editor to say if an image is expandable so that editors don't have to memorize the syntax for expandable images in order to add an expandable image. It adds the syntax for them.
Screen Shot 2021-03-03 at 9 20 40 PM

Makes expandable images work in the lesson editor so you can preview them there.
Screen Shot 2021-03-03 at 9 29 49 PM

Testing story

  • Tested manually
  • Updated existing tests for impacted components

Follow-up work

Do we want to make expandable images work for the ScriptOverview or CourseOverview?

PR 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

@dmcavoy dmcavoy changed the title Add expandable checkbox and more places where expandable images work Improvements to editing with expandable images Mar 4, 2021
@dmcavoy dmcavoy changed the title Improvements to editing with expandable images Improvements to editing expandable images Mar 4, 2021
@dmcavoy dmcavoy requested a review from a team March 4, 2021 23:05
Copy link
Contributor

@Hamms Hamms left a comment

Choose a reason for hiding this comment

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

couple of minor nits, but overall LGTM! I love the idea of making sure we add explicit UI support for all of our custom rich text features

Comment on lines 50 to 51
let param = expandable ? 'expandable' : '';
let e = {target: {value: this.props.markdown + `\n\n![${param}](${url})`}};
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
let param = expandable ? 'expandable' : '';
let e = {target: {value: this.props.markdown + `\n\n![${param}](${url})`}};
const param = expandable ? 'expandable' : '';
const e = {target: {value: this.props.markdown + `\n\n![${param}](${url})`}};

type="checkbox"
checked={this.state.expandable}
style={styles.checkbox}
onChange={() => this.setState({expandable: !this.state.expandable})}
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this should be something like

Suggested change
onChange={() => this.setState({expandable: !this.state.expandable})}
onChange={(e) => this.setState({expandable: e.target.checked})}


ReactDOM.render(
<Provider store={getStore()}>
<ExpandableImageDialog />
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a specific reason this can't just go in edit-container alongside LessonEditor above?

<SafeMarkdown markdown={lesson.preparation} />
<EnhancedSafeMarkdown
markdown={lesson.preparation}
expandableImages={true}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can just do

Suggested change
expandableImages={true}
expandableImages

here and elsewhere

@dmcavoy dmcavoy merged commit cc99b8b into staging Mar 5, 2021
@dmcavoy dmcavoy deleted the expandable-image-follow-up branch March 5, 2021 02:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants