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

Replace all uses of the inline style syntax with alternatives #38786

Merged
merged 3 commits into from Jan 29, 2021

Conversation

Hamms
Copy link
Contributor

@Hamms Hamms commented Jan 28, 2021

CurriculumBuilder supports a system of using markdown to define arbitrary attributes on rendered elements. This is obviously something we can't support for security reasons, but fortunately it seems like the curriculum authors were mostly using it to add styles to images.

In this PR, I identify all the places in the content copied over from CB where we are attempting to use this unsupported syntax, and in each case I replace the syntax with some alternative that we do support. The use cases generally fall into one of four categories:

  1. Images being sized up, to support displaying them to students as instructional aids.
  2. Images being sized down, to help them work better as inline visual aids
  3. Images being floated, also to help them work better as inline visual aids
  4. A border being added to slides in tables

The solutions applied are as follows:

  1. Replace the inline image with an expandable thumbnail, and update the instructions to specify that the teacher can click on the thumbnail to view the full image
  2. Image was manually resized in an image editor
  3. I used our existing divclass syntax to wrap the images in appropriate Bootstrap class for floating
  4. I added some CSS to style all slides in all tables in all lesson plans automatically.

Testing story

Most changes were made through the levelbuilder interface, and all changes were visually verified.

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

@Hamms Hamms requested a review from a team as a code owner January 28, 2021 21:59
Copy link
Member

@davidsbailey davidsbailey left a comment

Choose a reason for hiding this comment

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

LGTM!

* precise way to implement this.
*/
img:only-child {
border: 1px solid $black;
Copy link
Member

Choose a reason for hiding this comment

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

this selector looks like it will affect the right things for now, but makes me a little nervous that it might start adding styles unexpectedly if something changed in the future (e.g. we add an image somewhere in our resources editor table on the lesson edit page). one idea for the future would be to add a css classname the outer divs of our react components which render markdown, and then scope any CSS styles specific to markdown to only take effect in divs with this classname.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, that sounds great! I could probably do that as a quick follow-up work item tomorrow

@Hamms Hamms merged commit 67f8749 into staging Jan 29, 2021
@Hamms Hamms deleted the replace-inline-styles branch January 29, 2021 17:33
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

3 participants