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

Make the image remove option work #3837

Merged
merged 7 commits into from
Nov 23, 2022

Conversation

rtibbles
Copy link
Member

@rtibbles rtibbles commented Nov 22, 2022

Summary

Description of the change(s) you made

  • Ensures that images get properly cleared when using the 'remove' option from the context menu
  • Create remove event mechanism for clearing custom span elements for custom markdown fields.
  • Consolidates all blank space wrapping in custom markdown field logic.
  • Cleans up squire keydown handling.
  • Modifies prop values using web component interface rather than directly modifying a component prop
  • Tweak markdown conversion to prevent addition of excess line breaks, which would stop images and formulae from appearing inline with each other

Manual verification steps performed

  1. Added an image to an exercise question.
  2. Pressed close.
  3. Reopened the question for editing, used the context menu to remove the image.
  4. Used arrow keys to navigate around the editor window
  5. Added line breaks and removed line breaks between images

Screenshots (if applicable)

Screencast.from.11-22-2022.02.28.17.PM.webm
Screencast.from.11-23-2022.02.31.18.PM.webm

References

Fixes #3817

@rtibbles rtibbles requested a review from bjester November 22, 2022 22:33
Copy link
Member

@bjester bjester left a comment

Choose a reason for hiding this comment

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

I'm still noticing some differences with this method vs with the cursor next to the image and inputting backspace. It seems that this still leaves an image wrapper, and that becomes a pesty blank space that will only be removed if you explicitly select it with the cursor and input backspace, or close and reopen the exercise.
Screenshot from 2022-11-22 15-56-55

@rtibbles
Copy link
Member Author

Oh, interesting - I guess the existing destroy wasn't doing enough. Will look in more detail!

@rtibbles
Copy link
Member Author

So, I've managed to make the span delete itself, but the   on either side that both get deleted with a simple backspace are still not removed.

@rtibbles
Copy link
Member Author

OK, this should all be working as intended now. I've updated the PR description with the additional fixes and an updated screencast.

Copy link
Member

@bjester bjester left a comment

Choose a reason for hiding this comment

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

LGTM!

@pcenov
Copy link
Member

pcenov commented Dec 6, 2022

@rtibbles I was able to identify the following issues:

  1. The images are not displayed in Kolibri once the channel has been imported there:
    2022-12-06_13-48-15
  2. After I remove an image and then I click again in the editor the image reappears. Happens in both Chrome and Firefox:
2022-12-06_15-10-43.mp4

@rtibbles
Copy link
Member Author

rtibbles commented Dec 6, 2022

The images are not displayed in Kolibri once the channel has been imported there:

For this, how was this image added? Was it pasted into the text box, added using the 'add image' button?

After I remove an image and then I click again in the editor the image reappears. Happens in both Chrome and Firefox:

Interesting, this must be an issue with the data not being persisted properly.

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.

3 participants