Have a new patch based on Paul's previous review.
Thanks,
Jason
http://codereview.appspot.com/4641058/diff/1/content/samplecontainer/examples/commoncontainer/cconviews.js
File content/samplecontainer/examples/commoncontainer/cconviews.js
(right):
http://codereview.appspot.com/4641058/diff/1/content/samplecontainer/examples/commoncontainer/cconviews.js#newcode29
content/samplecontainer/examples/commoncontainer/cconviews.js:29: *
@param {string}
On 2011/07/07 16:03:37, Paul Lindner wrote:
{string=} for optional params here and throughout
Done.
http://codereview.appspot.com/4641058/diff/1/content/samplecontainer/examples/commoncontainer/cconviews.js#newcode140
content/samplecontainer/examples/commoncontainer/cconviews.js:140:
$("#dialog_" + dialog_counter).dialog({
On 2011/07/07 16:03:37, Paul Lindner wrote:
do we really need jquery here?
I use jquery since sample container already import jquery library and
it's easy to use.
http://codereview.appspot.com/4641058/diff/1/content/samplecontainer/examples/commoncontainer/cconviews.js#newcode298
content/samplecontainer/examples/commoncontainer/cconviews.js:298:
"ui-widget ui-widget-content ui-helper-clearfix
ui-corner-all").find(".portlet-header")
On 2011/07/07 16:03:37, Paul Lindner wrote:
I think all of these css classes here and throughout should have a
unique prefix
so that they don't possibly clash. Bonus points if you can make the
prefix
configurable..
these are the same css classes used in sample container's
viewController.js, I prefer not to change the viewController code.
http://codereview.appspot.com/4641058/diff/1/features/pom.xml
File features/pom.xml (right):
http://codereview.appspot.com/4641058/diff/1/features/pom.xml#newcode9
features/pom.xml:9: * with the License. You may obtain a copy of the
License at
On 2011/07/07 16:03:37, Paul Lindner wrote:
chunk mismatch here.. weird..
will have a new patch and hope it doesn't have this issue
http://codereview.appspot.com/4641058/diff/1/features/src/main/javascript/features/views/viewenhancements-container.js
File
features/src/main/javascript/features/views/viewenhancements-container.js
(right):
http://codereview.appspot.com/4641058/diff/1/features/src/main/javascript/features/views/viewenhancements-container.js#newcode67
features/src/main/javascript/features/views/viewenhancements-container.js:67:
* @param {Object}
On 2011/07/07 16:03:37, Paul Lindner wrote:
Object.<string, string|Object>=
Done.
http://codereview.appspot.com/4641058/diff/1/features/src/main/javascript/features/views/viewenhancements-container.js#newcode85
features/src/main/javascript/features/views/viewenhancements-container.js:85:
if (orig_site != undefined && orig_site.getActiveGadgetHolder() !=
undefined){
On 2011/07/07 16:03:37, Paul Lindner wrote:
prefer !== form where possible.
Done.
http://codereview.appspot.com/4641058/diff/1/features/src/main/javascript/features/views/viewenhancements-container.js#newcode150
features/src/main/javascript/features/views/viewenhancements-container.js:150:
* url: URL to a web page to open in a URL site in the
container. (Note this should not
On 2011/07/07 16:03:37, Paul Lindner wrote:
bad jsdoc format. Try running gjslint to get some help.
Done.
http://codereview.appspot.com/4641058/diff/1/features/src/main/javascript/features/views/viewenhancements-container.js#newcode155
features/src/main/javascript/features/views/viewenhancements-container.js:155:
* @param {string}
On 2011/07/07 16:03:37, Paul Lindner wrote:
string= here and throughout for opt_ params
Done.
http://codereview.appspot.com/4641058/diff/1/features/src/main/javascript/features/views/viewenhancements.js
File features/src/main/javascript/features/views/viewenhancements.js
(right):
http://codereview.appspot.com/4641058/diff/1/features/src/main/javascript/features/views/viewenhancements.js#newcode22
features/src/main/javascript/features/views/viewenhancements.js:22:
On 2011/07/07 16:03:37, Paul Lindner wrote:
These are probably not needed if you have the deps set up correctly.
remove the gadgets['views'] = gadgets['views'] || {};
http://codereview.appspot.com/4641058/diff/1/features/src/main/javascript/features/views/viewenhancements.js#newcode66
features/src/main/javascript/features/views/viewenhancements.js:66: *
@param {string}
On 2011/07/07 16:03:37, Paul Lindner wrote:
{string=}
Done.
http://codereview.appspot.com/4641058/diff/1/features/src/main/javascript/features/views/viewenhancements.js#newcode89
features/src/main/javascript/features/views/viewenhancements.js:89: *
@param retValue:
On 2011/07/07 16:03:37, Paul Lindner wrote:
Type? {Object}?
Done.
http://codereview.appspot.com/4641058/diff/1/features/src/main/javascript/features/views/viewenhancements.js#newcode100
features/src/main/javascript/features/views/viewenhancements.js:100: *
@param resultCallback:
On 2011/07/07 16:03:37, Paul Lindner wrote:
{function} here I believe.
Done.
http://codereview.appspot.com/4641058/
