Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

improved text editor asset handling #1324

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
2 participants
Contributor

brewster1134 commented Sep 18, 2012

I have been having issues with optional assets with ckeditor & wysihtml (duplicate assets, uncompiled assets in prouction, etc.). this commit better leverages the asset pipeline for these assets.

I only made these changes for the bootstrap-wysihtml5 editor, but if it all looks good, the same updates could be made to ckeditor and any other vendored gem assets.

comments are greatly appreciated!

Contributor

brewster1134 commented Sep 18, 2012

the CI failure seems unrelated to my commits. my tests passed using CI_ORM=mongoid. anyone got an answer on that one? :)

Collaborator

mshibuya commented Sep 19, 2012

I'm sorry but I don't see any necessity for changing current lazy loading method.
Your change will bloat rails_admin.css and rails_admin.js so much.
Please also refer to #1312.

@mshibuya mshibuya closed this Sep 19, 2012

Contributor

brewster1134 commented Sep 19, 2012

how does this bloat the rails admin assets? it will only bundle the required assets if they are needed, and they will also be compressed and minified in production automatically.

Precompiling assets in development is a common way of testing assets for production. Suggesting to not precompile in development because of poor asset handling is not optimal IMO.

appending config.assets.precompile with the necessary assets on initialization would be an improvement, but its still not leveraging the asset pipeline like it could be. we can allow precompiling these assets, avoid duplicate initialization, and let the asset pipeline compress and minify all the needed assets by requiring them dynamically into rails_admin. I think being able to simply add the needed gem to Gemfile is a much cleaner implementation.

this is also an issue with ckeditor.

Collaborator

mshibuya commented Sep 19, 2012

Let's put aside minor resulting issues here. Your change is not the only way to solve them.
The point here is 'tight coupling' vs 'loose coupling'.

What you are proposing is 'tight coupling'.

  • Upsides are:
    • simplifies configuration, because RailsAdmin takes care of most part of setup
  • Downsides are:
    • rails_admin.js and rails_admin.css will get larger, because bootstrap-wysihtml5 assets will be contained in there on compilation. It might not be big deal because bootstrap-wysihtml5 is relatively small, but what if we go for it with CKeditor, CodeMirror, and so on?
    • bootstrap-wysihtml5 assets can't be reused in non-admin part.
    • adds overhead on assets compilation and loading of rails_admin.js and rails_admin.css, even if only non-admin part of app needs bootstrap-wysihtml5 and RailsAdmin do not use it at all.
    • reduces flexibility. What if we want to use some CDN-hosted version of bootstrap-wysihtml5?

Considering all these point, I believe current 'loose coupling' strategy should be our way to go.
What do you think?

Contributor

brewster1134 commented Sep 19, 2012

Its certainly not the only way to solve the problem but i like the approach of consolidating asset dependencies, similar to bundler or sprockets. the current implementation can be problematic but makes sense if asset overhead is a priority. I suppose it comes down to a matter of preference, and you seem to have more vested in rails_admin than I.

maybe declaring something like config.text_editor = :ckeditor can be used to require assets or append to config.assets.precompile instead of just checking that a gem is included? at least the configuration for rails_admin would be consolidated in one place. just a thought.

to the point of reusing editor assets in non-admin parts... isn't that currently a limitation?

you certainly make valid points and i enjoy debating these types of issues 😄

Collaborator

mshibuya commented Sep 20, 2012

Current implementation allows reuse of bootstrap-wysihtml5 assets if you link them separately. And can't be reused if you go with asset-pipeline style consolidation, as you have pointed out.

Thanks for your effort to improve RailsAdmin 😺

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment