-
Notifications
You must be signed in to change notification settings - Fork 13
Adds Ember CLI Fastboot 1.0.0 compatibility #2
Conversation
Hi @willviles! Thanks for the awesome PR! I will review it ASAP. |
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.
👍
index.js
Outdated
let jsPath = path.join(vendorPath, 'medium-editor', 'js'); | ||
let stylesPath = path.join(vendorPath, 'medium-editor', 'css'); | ||
// Setup paths | ||
const vendorPath = this.treePaths.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.
@willviles I prefer to use let
inside of functions according to this rule. Any advantages for using const?
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.
Interesting - I've always used const
for values I know that won't change in that function scope. But that link does clarify it for me. Thanks!
package.json
Outdated
"ember-cli-babel": "^6.0.0-beta.3", | ||
"medium-editor": "^5.23.0" | ||
}, | ||
"devDependencies": { | ||
"broccoli-asset-rev": "^2.4.5", | ||
"broccoli-funnel": "^1.1.0", |
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.
@willviles I am not sure if this should be devDependecies
or dependencies
. Is there case when ember app do not have broccoli-merge-trees
, broccoli-stew
or broccoli-funnel
as dependencies? If so then we should include them as dependencies
.
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.
Hmm, yes that's an oversight of mine. Of course they need to be in dependencies
!
index.js
Outdated
if (!options.excludeStyles) { | ||
// Import styles | ||
if (!this.addonConfig.excludeStyles) { | ||
const theme = this.addonConfig.theme; |
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.
@willviles one more const :)
index.js
Outdated
}, | ||
|
||
treeForVendor(vendorTree) { | ||
// do not include assets in the fastboot mode | ||
if (isFastboot()) { return vendorTree; } | ||
const moduleName = 'medium-editor'; |
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.
@willviles and the last one!
The following PR adds Ember CLI Fastboot 1.0.0 compatibility, as discussed in #387: Prep for fastboot 1.0.0.
It follows the new importing pattern outlined here.
@kolybasov, I've also added issues to both my plugins Ember Medium Editor Insert and Ember Medium Editor Button to refactor the plugins away from the seemingly defunct Ember CLI Medium Editor to be compatible with this addon.
Whilst refactoring, I'll ensure both are Fastboot 1.0.0 compatible too.