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

Append build folder to output #740

Merged
merged 3 commits into from
Aug 26, 2015
Merged

Append build folder to output #740

merged 3 commits into from
Aug 26, 2015

Conversation

taylortom
Copy link
Member

Fixes: #735

@@ -33,7 +33,9 @@ AdaptOutput.prototype.publish = function (courseId, isPreview, request, response
tenantId = user.tenant._id,
outputJson = {},
isRebuildRequired = false,
// should this be Constants.Defaults.ThemeName?
Copy link
Member

Choose a reason for hiding this comment

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

I think you're correct.

@dennis-learningpool
Copy link
Member

Hey Tom - I think this is required for Framework 2.0 integration, right? Can you link an issue and update the description? Thanks.

@taylortom
Copy link
Member Author

Cool, I'll sort that now.

Another minor question: I've appended the build folder to the output, but is this actually needed? it doesn't look like anything else goes in there atm?
i.e. path.join(Constants.Folders.AllCourses, tenantId, courseId, Constants.Folders.Build));
or: path.join(Constants.Folders.AllCourses, tenantId, courseId));

@brian-learningpool
Copy link
Member

I don't think the build folder should be appended to the output, unless something else has changed in the framework?

@brian-learningpool
Copy link
Member

+1

1 similar comment
@dennis-learningpool
Copy link
Member

+1

dennis-learningpool added a commit that referenced this pull request Aug 26, 2015
Append build folder to output
@dennis-learningpool dennis-learningpool merged commit 062fe54 into develop Aug 26, 2015
@brian-learningpool brian-learningpool deleted the issue735 branch October 16, 2015 14:39
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.

3 participants