-
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
Rating tool integration btp #268
Conversation
exp_build/exp_gen.js
Outdated
@@ -4,6 +4,8 @@ const shell = require("shelljs"); | |||
|
|||
const { Experiment } = require("./experiment.js"); | |||
const Config = require("../config.js"); | |||
const { Plugin } = require("./plugin.js"); | |||
const { PluginConfig, PluginScope } = require("../enums.js"); |
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.
All imports from enums.js can be combined in one statement
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.
Done, removed unnecessary imports as well
js_modules: [ | ||
"./index.js", | ||
"./config.js", | ||
"https://apis.google.com/js/api.js", |
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.
Why do we need api.js?
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 api js script was present in the html of svc-rating tool as well. It is used for loading and initializing the Google API client library,
} | ||
}, | ||
{ | ||
id: "svc-rating", |
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.
Why is the ratings config different between testing and prod? It should be the same
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 think the testing one is correct. The prod one is based on the old approach of adding plugins to page.
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 prod one. Had taken the prod one based on the old PR
expScopePlugins.forEach((plugin) => { | ||
try { | ||
shell.cd("plugins"); | ||
prepareRepo(plugin); |
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.
Why is the call to prepareRepo removed?
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 method was created in the Plugin class called loadAllPlugins which is called inside the build method of experiment class. Inside loadAllplugins, prepareRepo is called.
"https://cdnjs.cloudflare.com/ajax/libs/font-awesome/6.2.1/css/all.min.css", | ||
], | ||
attributes: { | ||
spreadsheetID: "1x12nhpp0QvnsA6x-O1sV4IA9SAbfVsq_wiexWkutOmU", |
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.
Why do we need spreadsheetID here? Isn't it passed as argument to the component
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 handlebar for rating display component takes the attributes specified in the config file. The rating display handlebar then gets rendered in the html pages.
<ul class="navbar-nav ml-auto text-center d-flex flex-md-row"> |
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.
has the navbar been replaced by the ratings components?
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 earlier navbar.html was static i.e. it didn't used to get built from a handlebar. Since we wanted to have the rating display component on it we create a handlebar for navbar where we have passed the rating component as well as the links for HOME, PARTNERS and CONTACT as list items similar to the way in the old navbar. The resultant navbar looks the same as before we integrated the rating component.
exp_build/plugin.js
Outdated
@@ -31,20 +31,55 @@ function setCurr(component, targetPath, subTaskFlag = false) { | |||
return { ...obj, isCurrentItem: isCurrentItem }; | |||
} | |||
|
|||
|
|||
function isURL(source) { |
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 duplicated, I think it is present in another file as well. Please check
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 it was duplicated I fixed it
Integrated svc-rating-tool for experiment and lab pages.