-
Notifications
You must be signed in to change notification settings - Fork 14
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
Render json with latex #107
Conversation
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.
Overall the structure looks better than the older structure. We may need a few iterations though.
Please look at the review comments and resolve
Experiment.js
Outdated
@@ -61,6 +55,21 @@ class Experiment { | |||
shell.mkdir(path.resolve(this.src, Config.Experiment.build_dir)); | |||
shell.cp("-R", path.resolve(this.src, Config.Experiment.exp_dir), bp); | |||
shell.cp("-R", path.resolve(Experiment.ui_template_path, "assets"), bp); | |||
|
|||
// Copy the Katex CSS and fonts to the build directory in katex_assets | |||
shell.mkdir(path.resolve(bp, "katex_assets")); |
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.
Now that we are panning to have an asset directory, maybe we can put the KaTeX assets inside /asset/katex directory
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.
We have moved katex_assets to the assets directory
Task.js
Outdated
return final_paths; | ||
} | ||
|
||
cssPath() { |
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.
Both jsPath and cssPath can be the same function with final_path and js_path/css_path passed in as parameters.
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.
A new function is created by the name finalPath that takes 1 parameter
plugin.js
Outdated
|
||
if(!options.isValidate) | ||
{ | ||
pluginConfig = pluginConfig.filter((p) => p.id !== "build-validation"); |
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.
Should the id change to tool-validation?
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.
Yeah, we have changed it to tool-validation
plugin.js
Outdated
|
||
if(!options.isValidate) | ||
{ | ||
pluginConfig = pluginConfig.filter((p) => p.id !== "build-validation"); |
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.
Should the id change to tool-validation?
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.
Yeah, we have changed it to tool-validation
validation/schemas/learningUnit.json
Outdated
} | ||
{ | ||
"$schema": "http://json-schema.org/draft-07/schema#", | ||
"$id": "https://virtual.labs/schemas/learningUnit.json", |
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 do not see the js-modules and css-modules properties added here
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.
Added now
Task.js
Outdated
} | ||
|
||
const absolute_path = path.resolve( | ||
path.join(Config.build_path(this.exp_path), this.basedir, js_path) |
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.
What happens if the path.resolve returns an error, in case the path mentioned is not a valid path in the system?
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.
path.resolve will return a relative path to that file, after that, we use fs.existsSync to check whether the file actually exists or not.
new URL(source); | ||
return true; | ||
} catch (e) { | ||
return false; |
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.
Please log a statement here mentioning that ${source} is not a valid URL. Will help in case an intended URL has errors in it
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.
A log statement is added
…abs/ph3-lab-mgmt into render-json-with-latex
This reverts commit 359a267.
…lab-mgmt into render-json-with-latex
Experiment.js
Outdated
let paths = getAssessmentPath(nextSrc, unit.units); | ||
assessmentPath.push(...paths); | ||
} | ||
if (unit["content-type"] === "assesment" || unit["content-type"] === "assessment") { |
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.
You should use the enum defined in enums.js here instead of the string "assessment" for comparison.
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.
changed, now using content types from enums.js
Logger.js
Outdated
const { format } = winston; | ||
const { combine, timestamp, printf, colorize } = format; | ||
|
||
const myFormat = printf(({ level, message, timestamp }) => { |
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.
Can we think of a better name like vlabsDefaultFormat or even defaultFormat?
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.
updated, the new name is vlabsDefaultFormat
No description provided.