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
| :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.
Fixes #1446