-
Notifications
You must be signed in to change notification settings - Fork 8
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
267/display data in classrooms #275
Conversation
@pyphilia, a small comment on the layout of the list of Actions and Resources itself, I think that the delete icon should be on the right. Also, can we use primary colours for icons? Or maybe a grayish one. These black ones are a bit harsh. |
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.
Very nice! Almost wrapped up for release. Just some minor changes.
// update data | ||
const now = new Date(); | ||
classroom.assign({ name, updatedAt: now }).write(); | ||
classroom.assign({ name, lastUpdatedAt: now }).write(); |
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 believe it should be updatedAt
, right? We change it last time from this.
const logger = require('../logger'); | ||
|
||
// @param deleteSelection : map key-value with space id as key to whether the space data should be deleted |
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 be in JSDoc comment format.
name, | ||
deleteSelection, | ||
}; | ||
})(); |
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.
Do you actually need to call the function with the last ();
bit? I don't remember it like that in the other classes.
<> | ||
<Typography className={classes.dataTitle}>{t('Spaces')}</Typography> | ||
|
||
{spaces.length > 0 ? ( |
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.
spaces.length
no need for > 0
|
||
// username cannot be null if actions or resources are imported | ||
if (!username && (appInstanceResources || actions)) { | ||
toastr.error(ERROR_MESSAGE_HEADER, 'A user need to be specified'); |
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.
needs
. Do we factor out and translate?
@@ -0,0 +1,55 @@ | |||
export const isClassroomNameValid = name => { |
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 might want to have simple, but specific unit tests for these functions.
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.
There is one tiny change I suggest. But then you can go ahead and merge.
src/utils/classroom.js
Outdated
return name.trim().length; | ||
|
||
// check name is a string | ||
if (typeof name !== 'string' && !(name instanceof String)) { |
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 would recommend using Lodash. https://lodash.com/docs#isString
close #267