-
Notifications
You must be signed in to change notification settings - Fork 13
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 logic to toggle reducer keys based off user input #1610
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.
Hey Toyosi!
Overall this looks good. But I'm unsure about having a "global" forms.js that will load in all our forms but whose logic will only work for the reducers form. I think we can have a <script> tag appended at the bottom of _form.html.erb that can deal with this logic, so that we can limit the scope.
If you are familiar with jQuery, we have access to jQuery in our rails app. (I'm a fan, but totally ok with vanilla JS)
I think for the reducer's_form.html.erb
instead of using class='hidden'
as initial default for subject_reducer_keys, we can do the following:
L18-25 of reducer's _form.html.erb we can declare the divs and form inputs. i.e.
<div id='user_reducer_keys'>
<%= f.input :user_reducer_keys%>
</div>
<div id='subject_reducer_keys'>
<%= f.input :subject_reducer_keys%>
</div>
And then we can add a <script>
tag on the bottom of reducer's _form.html.erb that can hide and show elements on load and on change. I.e.
<script type="text/javascript">
function toggle_reducer_keys_inputs(topic) {
if (topic.includes('subject')){
$('#subject_reducer_keys').hide()
$('#user_reducer_keys').show()
} else {
$('#subject_reducer_keys').show()
$('#user_reducer_keys').hide()
}
}
$(function() {
var reducer_topic = $('#reducer_topic').val();
toggle_reducer_keys_inputs(reducer_topic)
});
$('#reducer_topic').on('change', function(event){
var reducer_topic = event.target.value;
toggle_reducer_keys_inputs(reducer_topic)
})
</script>
Something like that...it's been a hot sec since I've done any jQuery/javascript stuff, but I'm hoping that should work out 🤣
@@ -0,0 +1,36 @@ | |||
document.addEventListener("DOMContentLoaded", function () { |
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 question whether or not we need a form.js, because technically this javascript will load on all our forms, but the logic will only run for reducer form.
I think it might be better to have a <script>
tag with your logic within the reducer's form partial (views/reducers/_form.html.erb).
Luckily we also have access to jquery, but I dont mind vanilla js as well :)
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, using script makes sense aswell, i was thinking views/forms incase we need other/extra form logic in future but this flies. I've modified the logic to use jquery now and it works just as you shared, vanilla js usuallly comes natural to me by default most times 😊
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.
ooh nice!! i literally just free coded that from what little i remember of js + jquery without testing whether or not that would work. 😆 .
@@ -131,4 +137,15 @@ def reducer_params(klass) | |||
def record_not_valid(exception) | |||
render json: { error: exception.message }, status: 422 | |||
end | |||
|
|||
def remove_unwanted_config_key(param_object) |
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 find the name of this method a bit confusing. Maybe we can call it reset_config_reducer_keys(param_object)
?
user_reducer_keys: 'user_reducer_keys_value', | ||
subject_reducer_keys: 'subject_reducer_keys', | ||
topic: 'reduce_by_subject' | ||
}, |
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.
Dont forget to address the hound 🐶.
Hound comment for this one before was
Style/TrailingCommaInHashLiteral: Avoid comma after the last item of a hash.
Solves for this ticket