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

Add support for adapt_framework -> grunt server-build:dev #857

Merged
merged 35 commits into from
Nov 25, 2015

Conversation

taylortom
Copy link
Member

Partial fix for #811

darylhedley and others added 14 commits April 15, 2015 15:47
This was due to the default framework course folder being copied over to
the temporary working/build directory.  The fix is to remove it on
installation.
Adds ifUserIsMe and fixes inverse not being passed back
Clarified NodeJS version.
Version 0.1.1.
Add a Gitter chat badge to README.md
@taylortom
Copy link
Member Author

Also see adaptlearning/adapt_framework#734

@brian-learningpool
Copy link
Member

@taylortom, I really like this. Source maps would be of major help to developers trying to debug client-side code. The only thing I have issue with is using the isProduction config to toggle this. This config is already used in determine whether the web application should run in production mode.

Perhaps a new course setting would be more applicable?

@taylortom
Copy link
Member Author

Fine by me - are you thinking another config.json option? (e.g. generateOutputSourcemap)

@brian-learningpool
Copy link
Member

Yes, either in Configuration Settings or Project Settings.

@taylortom
Copy link
Member Author

@brian-learningpool this work should be done now

@taylortom
Copy link
Member Author

TODO: test this in older versions of the framework to make sure the params don't cause any unexpected problems if not used.

@brian-learningpool
Copy link
Member

@taylortom, this is really useful. I've just been testing this locally and I think there's one small thing it's missing though. When the _generateSourcemap value changes you you should trigger a flag to indicate that a full rebuild is required. Have a look at the rebuildCourse event:

 app.emit('rebuildCourse', tenantId, courseId);

}
}
}
},

Choose a reason for hiding this comment

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

usually wary of trailing commas in a JS array like this - is it ok?

Copy link
Member Author

Choose a reason for hiding this comment

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

Think I ran the jscs grunt task at some point which may have added this.

@taylortom
Copy link
Member Author

+1 for @brian-learningpool's changes

@brian-learningpool
Copy link
Member

+1 for yours too! 👍

@dennis-learningpool
Copy link
Member

+1 for everyone!

brian-learningpool added a commit that referenced this pull request Nov 25, 2015
Add support for adapt_framework -> grunt server-build:dev
@brian-learningpool brian-learningpool merged commit 951736f into develop Nov 25, 2015
@brian-learningpool brian-learningpool deleted the issue/811 branch November 25, 2015 15:51
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.

5 participants