Set default values for min/max handler #4627
Conversation
| } else { | ||
| const defaultVal = dim == 'min' ? 1 : 999; | ||
| changeElement.attr(dim, defaultVal); |
There was a problem hiding this comment.
I love the precision here in making just a small enough change, but sadly, I feel like it's not enough.
It seems to me that we should embed the min or max to fall back on. Take this for example, we should fall back to these values if they're defined.
some_element:
widget: number_field
min: 2
max: 4If we can't fallback to any values, then we just don't. I think the conservative route of just doing nothing is better than choosing 1 or 999. But we can document that these are fallback values when you use data-min or data-max sparingly.
| :div, | ||
| id: [form.object_name, attrib.id, 'wrapper'].join('_'), | ||
| class: attrib.hide_by_default? ? 'd-none' : '', | ||
| data: data |
There was a problem hiding this comment.
Seems like we need should have some sort of helper function as a member function of the smart attribute to generate this hash. That way it can deal with all the edge cases and it encapsulates that logic within that object.
There was a problem hiding this comment.
@Bubballoo3 FYI - let me know if a member function on the smart attribute parent class is to your liking. I think it makes sense there as it'll likely need all sorts of toggles based on the widget type.
There was a problem hiding this comment.
Yeah I think that would be good. We should try to keep this as small as possible though
There was a problem hiding this comment.
If it were me, I'd likely put it all in a module that you include so that it's all encapsulated in one single file. In that way, it won't be mixed in with other functionality when you try to read and understand it. It's one file that serves one purpose, even if it has many lines. But that's just me.
There was a problem hiding this comment.
Wouldn't we only want a module if we need to use it elsewhere in ruby code? I might be misunderstanding; are you talking about just the data hash or would it do more?
There was a problem hiding this comment.
Wouldn't we only want a module if we need to use it elsewhere in ruby code? I might be misunderstanding; are you talking about just the data hash or would it do more?
React
No it wouldn't do more, but my thinking is it may be nice to just to have all this functionality & logic contained in 1 file instead of being spread out amongst all the other functionality and logic in the existing file.
It's not about reuse it's more about separating functionality.
| :div, | ||
| id: [form.object_name, attrib.id, 'wrapper'].join('_'), | ||
| class: attrib.hide_by_default? ? 'd-none' : '', | ||
| data: data |
There was a problem hiding this comment.
Yeah I think that would be good. We should try to keep this as small as possible though
| min_val = attrib.opts[:min] | ||
| max_val = attrib.opts[:max] | ||
| data[:min_default] = min_val if min_val | ||
| data[:max_default] = max_val if max_val |
There was a problem hiding this comment.
I think storing the min/max here might add a bit more complexity than we need, as it requires us to keep conventions between server-side and client-side code, which is difficult as we can't share any methods between ruby and javascript. We can expand this initial hash whenever necessary ofc, but things like mins and maxes shouldn't need to be collected at this stage since we know the widget will be ok when it renders. IMO it would be much easier to manage if all of this was handled in dynamic_forms.js, and we load the default values into the data hash whenever we bind the min/max-for listener. This also prevents us from expanding the data hash for static widgets (without min/max-for directives), where this info is unnecessary bloat.
Just to justify the existing items under this criteria, widget_type is necessary because we don't have any other way of connecting the complex form items (like resolution_fields) to a meaningful name. The hide_by_default attribute is less necessary admittedly, but ok because we absolutely have to check hide_by_default? when we add the d-none class, and it is simpler to just get it from the attrib object here than detect it from the d-none class later.
Let me know what you guys think of this, obviously the wrapper is new so we could go several ways with what it becomes, and this current approach does reduce complexity in dynamic_forms, which is definitely already complex enough. Overall though I think trying to avoid creating conventions that have to be followed in two places will save us some headache as we use this more.
There was a problem hiding this comment.
We can expand this initial hash whenever necessary ofc, but things like mins and maxes shouldn't need to be collected at this stage since we know the widget will be ok when it renders. IMO it would be much easier to manage if all of this was handled in dynamic_forms.js, and we load the default values into the data hash whenever we bind the min/max-for listener.
I don't think I follow you. It needs to be rendered in the HTML on the server side somewhere at some point. But I could be completely missing what you're trying to say.
There was a problem hiding this comment.
I am saying that unless the data information needs to be rendered server-side for some reason, we should store and retrieve that data exclusively in the javascript, just to keep that storing and retrieval in a single place. Since we can get the min and max information from the html post-rendering, this would be a place where we could just as effectively gather and store min_default and max_default in addMinMaxForHandler and set the element's data in the same way, allowing us to only store data on widgets that we are going to change dynamically later, and keep min/max-for functionality entirely withing dynamic_forms.js.
There was a problem hiding this comment.
Oh I think I got you - because mins and maxes will be in the HTML before any of our javascript runs that's when & where we can extract it. I.e., the defaults are there before we modify them when our handlers run. Is that it?
There was a problem hiding this comment.
Yeah. And even though the resulting data attribute will be the same by the time we start changing the mins/maxes, we should still try and set this at the latest possible moment, since that is the closest to when the data will be used/accessed (same principle as initializing a variable close to where it's used). The most immediate impact is reducing the splash radius of this change to only dynamic_forms.js and not having to touch session_contexts_helper.rb
Fixes #1446