Add HTML wysiwyg editor with image upload

[6temes]

I just added trumbowyg and configured an endpoint to upload images.

It's still not production code -no tests nor documentation- but I wanted to know what you think about.

[jamesmk]

Thanks for the great PR @6temes! I like your choice of wysiwyg and integration method and would likely merge this PR with a few considerations addressed.

At a high level my main concern is the added page weight for a feature we won't be using a whole lot. It looks like core is about 20kb, which I'm not too concerned about but the PR includes a number of plugins and lang files as well. I'm not familiar with how much of that will be loaded by default.

A couple options for us here are:

• conditionally load lib when .js-html-editor is present on the page
• load lib only on form pages
• create a feature flag in the Fae initializer

I considered this same issue with our Markdown GUI as well. Let me know your thoughts

Also, if we don't adopt this feature internally there's the topic of maintenance. Features not commonly used by the core maintainers will get overlooked, so we'd have to lean on the community more to support this feature. Not a deal breaker but can be an awkward position.

Other than that, this would still require tests and docs as you mentioned.

thanks again!

[jamesmk]

While we have the ability to manage content in multiple languages in Fae, the interface only supports english currently. I'd prefer to keep all the lang files out and maybe supply a info in the docs to users can add them to the app if they so wish.

[6temes]

In order to conditionally load the libraries, I see two options:

The easiest way would be to load them in the app/assets/javascripts/fae.js (user's side). We would just have to add the commented out dependencies in the generator, and add a comment saying 'Uncomment if you want to use'.

The other way would be to configure them in the initializer.

As for the i18n, that's the next thing I wanted to do. If I have to use this CMS, I will need to internationalize it because I don't work in an English speaking country.

Also, if we don't adopt this feature internally there's the topic of maintenance. Features not commonly used by the core maintainers will get overlooked, so we'd have to lean on the community more to support this feature. Not a deal breaker but can be an awkward position.

I guess the way to go is that if someone needs a feature, that person/company will have to maintain it. At the same time, I guess that its wise to add only features that are relevant for a big subset of the users. If someone needs something really specific, that person have the option to maintain their own fork.

[6temes]

With the last commit, the trumbowyg libraries are opt-in.

I thought to do the same with the Markdown editor but then I thought that it would be a breaking change. What do you think, @jamesmk ?

The other way would be, as you said, to add a configuration flag in the initialized with defaults to SimpleMDE, but if don't think if it is really necessary.

[jamesmk]

I like your idea to move the inclusion of the libs to fae.js. I will consider doing this for all plugin like files for Fae 2.0. Thanks for removing the language files too. Might be worth adding instructions on how to add i18n support in the docs.

Think you're missing some tests and docs, then this is close to mergeable. @aaronransley: mind jumping in and taking a look as well?

Thanks for your work on this!

[6temes]

I will start to work in the documentation and tests. I just wanted to make sure that the code was ok before.

As for locales, I would like to add support for i18n to this project. In my case, I need Japanese locales, but I am sure that more people will want to add their locales. But well, this belongs to the next PR. ;)

[jamesmk]

@6temes Sorry yesterday we noticed a bad merge commit on master, so I had to rebase master to remove it. It looks like those commits are still on your branch (hench the now 38 commits this PR introduces). Can you rebase your branch to master? If that doesn't work, you may have to cherry-pick your commits to a new branch :/

thanks

[6temes]

Hi @jamesmk. No problem. Because I was not sure about what was meant to be changed, I just replaced the branch with a fresh one from the new master and cherrypicked my commits. It was easy. :)

Can you please check if the Changelog is correct?

[jamesmk]

Perfect, thanks 👍

[6temes]

@jamesmk Sorry for the delay, I have been working in other projects this days. I added the documentation.

I have been checking the current tests but I am not really familiar with feature tests. Do you have any idea about which part of the functionality would make sense to test?

[jamesmk]

To add feature tests you'll need to add an example in the dummy app. We'll want it there to QA the feature separate from any specific integration. Since we have some undocumented rules around the dummy app I can add the example and a couple feature tests around it.

Can you add a test for create_html_embedded to verify it returns the expected JSON? Also, can you resolve the Code Climate issues (https://codeclimate.com/github/wearefine/fae/pull/224)?

thanks

[6temes]

I created the test for the endpoint and fixed the Code Climate errors.

Regards!

[6temes]

(Sorry, I had a little accident while working on my fork)

@jamesmk What do you think about the tests I introduced. Do you think that this branch can be merged?

[jamesmk]

@6temes Looks good, thanks for your work on this PR! I'm releasing v1.5.1 now and will merge it after to include in v1.6.

[6temes]

Thanks!

[jamesmk]

fixes #213