-
-
Notifications
You must be signed in to change notification settings - Fork 928
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
SAK-49806 Rubrics show hidden rubric button to instructors #12572
base: master
Are you sure you want to change the base?
Conversation
@@ -110,5 +110,9 @@ export class RubricsElement extends SakaiElement { | |||
tr(key, options) { | |||
return super.tr(key, options, "rubrics"); | |||
} | |||
|
|||
isAttributeReallyTrue(property) { |
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 don't think we need this method lets just do the proper check when needed
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, I kind of prefer the === here.
You might be able to use lodash isEqual method if you wanted it to be cleaner. I'd be okay with that.
I'd think these would be identical and seem about the same.
this.instructor === "true"
import { isEqual } from 'lodash'
isEqual(this.instructor, "true")
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'll pivot back to using === for each instance and force push a revised commit.
instructor: { type: Boolean }, | ||
instructor: { type: 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've used this pattern of
instructor: { type: Boolean }
and have witnessed that it does work well.
I am just curious why here it does not?
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.
Prior to this question, I have assumed that the following HTML attribute setting is valid/intended for SakaiRubricStudentButton and SakaiRubricStudent.
instructor=”false”
If the instructor type is Boolean, the attribute above yields a value of true for the this.instructor property.
Is the intent instead not to display an instructor attribute for cases where this.instructor needs to be false? In other words, is the instructor attribute similar to "checked" for input[type="checkbox"]?
Otherwise, if the HTML case above of explicitly setting the attribute to "false" is valid/intended, we need the instructor property of type String to detect when the attribute is set to “false”.
(Though I haven't investigated the following very closely, maybe there was a behavior change for Element Lit between v1 and v2 for the Boolean property type's default behavior.)
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 did a few changes recently where I used
someVariable: { type: Boolean }
and it worked fine
I think your right though that it should be investigated as to why it is not working here
Jira: https://sakaiproject.atlassian.net/browse/SAK-49806
Instead of having an '=== true' conditional throughout this code, I placed it as a new method in RubricsElement.