-
Notifications
You must be signed in to change notification settings - Fork 1
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
Drop codemirror-rails and coffee => Rails 6 + 7 support #3
Conversation
Copied from https://github.com/fixlr/codemirror-rails/tree/master/vendor/assets commit cb5e9eee1bac8b8b6933d330591a5f83100094d8. This is identical to version "5.16.0" of "codemirror-rails".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but one issue
Also:
- Is the LGPL we currently use compatible with the included MIT? If unsure, we could switch to MIT.
@@ -2,7 +2,7 @@ Gem::Specification.new do |s| | |||
s.author = 'Scrivito' | |||
s.description = 'Make CodeMirror available as a Scrivito editor' | |||
s.email = '[email protected]' | |||
s.files = Dir['READ*', 'LIC*', 'app/**/*', 'lib/**/*'] | |||
s.files = Dir['READ*', 'LIC*', 'app/**/*', 'lib/**/*', 'vendor/**/*'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- The CodeMirror code looks like the addons rely on the main script to be loaded first. If I read this correctly, we should make the order explicit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is how codemirror-rails
defines its "files":
So I think we're ok with the current approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Ruby even turned to sorted at some point: https://bugs.ruby-lang.org/issues/8709
Since // CodeMirror, copyright (c) by Marijn Haverbeke and others
// Distributed under an MIT license: http://codemirror.net/LICENSE I think mixing them is good enough. But since |
https://github.com/fixlr/codemirror-rails is officially not supporting Rails 6 or Rails 7. But both rails versions still support the asset pipeline if needed.
This PR converts "coffee" to plain js to remove the coffee dependency.
It also inlines the "vendor/" part of
codemirror-rails
so that it can be directly used. So versionCodeMirror 5.16.0
is now included in the package itself.