Skip to content

Set default values for min/max handler #4627

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.

Sign up for GitHub

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Set default values for min/max handler #4627

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2cb2fed
Set default values for min and max
ahmed-mgd Sep 24, 2025
74793dd
Revert default value fallback
ahmed-mgd Oct 29, 2025
9d847ca
Merge branch 'master' into min-max-handlers-1446
ahmed-mgd Oct 29, 2025
9f28346
Fall back on default min/max values from wrapper
ahmed-mgd Oct 29, 2025
98ac95b
Revert "Fall back on default min/max values from wrapper"
ahmed-mgd Nov 5, 2025
fe20726
Get default values before attaching form handlers
ahmed-mgd Nov 7, 2025
0b26bcc
Remove debug log
ahmed-mgd Nov 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 11 additions & 4 deletions apps/dashboard/app/helpers/batch_connect/session_contexts_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,14 +42,21 @@ def create_widget(form, attrib, format: nil, hide_excludable: true, hide_fixed:
end

# Append a wrapper div to hold additional info
data = {
widget_type: widget,
hide_by_default: attrib.hide_by_default?
}

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
Copy link
Contributor

@Bubballoo3 Bubballoo3 Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@johrstrom johrstrom Oct 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@Bubballoo3 Bubballoo3 Oct 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@johrstrom johrstrom Oct 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@Bubballoo3 Bubballoo3 Oct 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


wrapped = content_tag(
:div,
id: [form.object_name, attrib.id, 'wrapper'].join('_'),
class: attrib.hide_by_default? ? 'd-none' : '',
data: {
widget_type: widget,
hide_by_default: attrib.hide_by_default?
}
data: data
Copy link
Contributor

@johrstrom johrstrom Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@johrstrom johrstrom Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

@Bubballoo3 Bubballoo3 Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think that would be good. We should try to keep this as small as possible though

Copy link
Contributor

@johrstrom johrstrom Oct 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@Bubballoo3 Bubballoo3 Oct 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@johrstrom johrstrom Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

) do
rendered
end
Expand Down
11 changes: 9 additions & 2 deletions apps/dashboard/app/javascript/dynamic_forms.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -553,12 +553,19 @@ function toggleMinMax(event, changeId, otherId) {
var ariaMsg = 'Set ';
var ariaMsgLength = ariaMsg.length;
[ 'max', 'min' ].forEach((dim) => {
const defaultVal = $(`#${changeId}_wrapper`).data(`${dim}Default`);
let newVal = undefined;
if(mm && mm[dim] !== undefined) {
changeElement.attr(dim, mm[dim]);
newVal = mm[dim];
} else if (defaultVal !== undefined) {
newVal = defaultVal;
}
if (newVal !== undefined) {
changeElement.attr(dim, newVal);
if (ariaMsg.length > ariaMsgLength){
ariaMsg += ' and ';
}
ariaMsg += `${dim}imum limit to ${mm[dim]}`;
ariaMsg += `${dim}imum limit to ${newVal}`;
}
});

Expand Down
Loading