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

Embed external images in Moments and Strategies editor #2205

Merged
merged 5 commits into from
Dec 8, 2022

Conversation

Rxthew
Copy link
Contributor

@Rxthew Rxthew commented Nov 19, 2022

Description

Implemented changes to allow editor to embed an image after the user provides an appropriate link via prompt.

More Details

This was implemented to conform with the rest of the workflow which uses pell.

One concern I have is that behind the scenes pell relies heavily on the document.execCommand property which is deprecated.

This is also highlighted as an issue in the repository itself.

Corresponding Issue

Issue 2141

Screenshots

Peek Issue 2141


Reviewing this pull request? Check out our Code Review Practices guide if you haven't already!

@welcome
Copy link

welcome bot commented Nov 19, 2022

Thank you for opening this pull request with us! Be sure to follow our Pull Request Practices. Let us know if you have any questions on Slack.

Copy link
Member

@julianguyen julianguyen left a comment

Choose a reason for hiding this comment

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

Great start, thanks so much for working on this 🎉

I pulled down your branch and the functionality works as expected!

A few things we need to address are making sure very large images are scaled to fix the container or else this will happen:

In the editor:

image

On the post page:

image

Through CSS, we want to set max-width: 100%;.

@Rxthew
Copy link
Contributor Author

Rxthew commented Nov 26, 2022

Great start, thanks so much for working on this 🎉

I pulled down your branch and the functionality works as expected!

A few things we need to address are making sure very large images are scaled to fix the container or else this will happen:

In the editor:

On the post page:

Through CSS, we want to set max-width: 100%;.

Hello,

Sorry this took so long, I've completed it this a while ago but I'm still experiencing issues with my dev environment.

I spent some time getting more familiar with the repository and I realised there was no dedicated component for posts so I added a class in the ERB files. This involved importing the containers from the core assets module into the global scss file, and I was a little uncomfortable with that so If there is a less disruptive approach I may have overlooked I could go back and make changes.

Copy link
Member

@julianguyen julianguyen left a comment

Choose a reason for hiding this comment

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

Thanks for the updates! Great work 🎉

@julianguyen julianguyen merged commit d9e34cd into ifmeorg:main Dec 8, 2022
@welcome
Copy link

welcome bot commented Dec 8, 2022

Thank you for merging this pull request with us! If you haven't already, in another pull request, please add yourself to our About page.

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.

2 participants