-
Notifications
You must be signed in to change notification settings - Fork 105
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
Issue281 - New activity setting: Automatic Recording #390
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.
I know you're still working on this, so I only did a brief check to see if I could understand the code.
5c9bca4
to
0401ecb
Compare
@aduranterres I rebased the code and rebuilt the JavaScript files with |
Hi @jrchamp Thank you very much for all the code cleaning you did, sorry about the unchanged headers, etc. I have uploaded some new changes:
The function definition_after_data is always executed after the form first loads, so that's why the second modification was needed. I see now that grunt is complaining again...I am not sure why as I have not changed them after I got your changes or run grunt again, so sorry for giving you more work. |
definition_after_data: don't deal with option_auto_recording if it is enabled
I was able to get a few minutes to apply your changes and I'm fairly sure that I have them all this time. 😄 @sgrandh3 Thank you for offering to test this. You will probably need one or two people to grant you scheduling privilege, so let me know and I'll grant you access. |
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.
Adding some comments about dynamic class properties and adjusting some admin-level setting defaults.
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.
The testing revealed a minor issues string issues that were easily addressed. Approving and merging. We'll plan to release this next week, hopefully with a performance improvement PR #401.
Resolves #281