-
Notifications
You must be signed in to change notification settings - Fork 4
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
Add note type for activities #1103
base: master
Are you sure you want to change the base?
Conversation
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.
Minor changes
Could only partially check because I can't read html/css and barely understand js and haml.
var par_notes; | ||
var notes = []; | ||
console.log(activity.has_notes_checkboxes); | ||
if (activity.has_notes_checkboxes()) { |
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 also know little JS, however, does it not have switch cases?
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.
Is there any way to define the enum in 1 place like in activity.rb
and reference it from the jabbascript?
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 think so, atleast not trivally
@@ -28,7 +28,8 @@ def create | |||
end | |||
|
|||
@member = Member.find(current_user.credentials_id) | |||
@notes = params[:par_notes] | |||
@notes = params[:par_notes] |
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 did the linter add a double space? Linters 🤷
var notes = []; | ||
console.log(activity.has_notes_checkboxes); | ||
if (activity.has_notes_checkboxes()) { | ||
console.log(this.notes.find(":checked")); |
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.
remove log or cache this.notes
call
'<div class="col-md-12">' + | ||
'<div class="input-group mb-3">' + | ||
'<span class="input-group-text">Title</span>' + | ||
'<input class="form-control" type="text" name="activity[notes_options][]">' + | ||
"</div>" + | ||
"</div>" + | ||
'<div class="col-md-6">' + | ||
'<div class="input-group mb-3">' + | ||
'<input class="form-control" type="text" name="activity[notes_options][]">' + | ||
'<div class="input-group-append">' + | ||
'<button class="btn btn-sm btn-danger delete-option" type="button">' + | ||
'<i class="fa fa-fw fa-minus"></i>' + | ||
"</button>" + | ||
"</div>" + | ||
"</div>" + | ||
"</div>"; |
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.
Perhaps add some indentation here?
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 preferable, however, an even better approach would be to eliminate the need for generating HTML within the JavaScript code altogether.
Make a HAML partial file as a template for the option menu.
In the _edit haml file render the template but keep it hidden with css attributes
Update the code that appends the options HTML to use the cloned template.
Modify the code that sets the input values to target the cloned elements and make it visible
Add an event listener for the "Add Option" button to clone and append the template.
Add an event listener for the "Delete Option" button to remove the corresponding option.
this is not required, but would prevent coding twice when changing the UI
@@ -184,12 +197,29 @@ Object.defineProperties(Activity.prototype, { | |||
*/ | |||
value: function (method) { | |||
var activity = this; | |||
var par_notes; | |||
var notes = []; | |||
console.log(activity.has_notes_checkboxes); |
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.
Shouldn't the console log be 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.
Yes
I wondered how long it would take to implement this without altering too much in the database. I don’t think this is necessarily the best way to implement it, but it seems fully functional at least.
Only the translations are missing, I think.