-
Notifications
You must be signed in to change notification settings - Fork 53
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
standardize cards [TASK-1158] #1186
Conversation
tiramisu24
commented
Oct 17, 2017
- refactor object and apps cards in narrative into separate functions
- standardize layout of cards
- move inline css into css files
…n content instead of just elipsis
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, looks pretty good. There's some things that should be addressed before merging, though.
@@ -2502,6 +2549,9 @@ button.kb-data-obj { | |||
} | |||
|
|||
.kb-data-list-info { | |||
width: 80%; | |||
padding: 4px 0px; | |||
border-top: 1px solid #E0E0E0;; |
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.
extra semicolon
* var $card = new kbaseDataCard( | ||
{ | ||
//expected values | ||
app: app, |
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 should the values be for the various keys here? Strings? Booleans? For the date should it be ms since epoch, or a pre-formatted string?
* creating function such as kbaseDataCard. | ||
* | ||
* Expected options: | ||
* options : { |
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.
As above, what are the various options expected to be? String, objects, etc.?
function KbaseCardLayout(options) { | ||
var self = this; | ||
//partitions | ||
var $card = $('<div/>', { 'class': 'narrative-card-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.
Don't need the trailing forward-slash in new jquery elements.
$ | ||
) { | ||
function KbaseAppCard(entry) { | ||
var self = entry.self; |
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 is 'entry.self' in this case? Another widget that has some attributes like a catalog client and several other options? This seems a little too dependent on passing in the calling object and expecting it to have things like the catalog client. Should probably just instantiate that here, as necessary.
var self = this; | ||
// add icon (logo) |
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.
Yay for tons of deleted code! 👍
.html($('<button class="btn btn-xs btn-default pull-right" aria-hidden="true">').append('<span class="fa fa-ellipsis-h" style="color:#888" />')); | ||
var toggleAdvanced = function () { | ||
if (self.selectedObject === object_info[0] && $moreRow.is(':visible')) { | ||
.append('<tr><th>Permament Id</th><td>' + object_info[6] + '/' + object_info[0] + '/' + object_info[4] + '</td></tr>') |
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.
Permanent Id , not Permament (second m -> n)
//$toggleAdvancedViewBtn.show(); | ||
|
||
if ($node.is(':visible')) { | ||
self.writtingLock = 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.
Might be a leftover from your previous work - writtingLock -> writingLock
@@ -90,7 +92,7 @@ define([ | |||
options: { | |||
title: 'Data', | |||
loadingImage: Config.get('loading_gif'), | |||
notLoggedInMsg: "Please log in to view a workspace.", | |||
notLoggedInMsg: 'Please log in to view a workspace.', |
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 will likely never be seen, but it's a good time to fix it. Should say "Please log in to view Narrative data."
@@ -396,11 +398,11 @@ define([ | |||
var user = Jupyter.narrative.userId; | |||
|
|||
if (!user) { |
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.
Wow, this never got updated when we moved to the new Auth platform. It tries to parse an old style auth token instead. You can safely remove this whole if 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.
Er, the if (!user)
block starting at line ~400