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
ahmed-mgd wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Set default values for min/max handler #4627

Changes from all 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
48 changes: 42 additions & 6 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 @@ -105,14 +105,43 @@ function snakeCaseWords(str) {
}

/**
*
* @param {HTMLElement} element
*/
function memorizeElement(element) {
formTokens.push(mountainCaseWords(shortId(element['id'])));
};

/**
*
* @param {HTMLElement} element
*/
function storeDefaults(element) {
const wrapper = document.getElementById(`${element.id}_wrapper`);
if (!wrapper) return;

['max', 'min'].forEach((dim) => {
const defaultVal = element.getAttribute(dim);
Copy link
Contributor

@Bubballoo3 Bubballoo3 Nov 7, 2025

Choose a reason for hiding this comment

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

I like the structure of this method and its error resistance, but I am not sure why you pass every form element through here when most do not have min/max defined. IMO it makes more sense to call this method in addMinMaxForHandlers so that

  1. Every element you try to store should have a max and min
  2. You only store elements that might be changed in the future

Copy link
Contributor Author

@ahmed-mgd ahmed-mgd Nov 7, 2025

Choose a reason for hiding this comment

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

Since addMinMaxForHandlers calls toggleMinMax at the very end, it ends up changing the default values for the "second" element. If/when we iterate to that second element later, we don't store the actual defaults for that element but those mutated values. I'm not sure if there's an easy way to work around this.

Copy link
Contributor

@Bubballoo3 Bubballoo3 Nov 7, 2025

Choose a reason for hiding this comment

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

Just to use the language from the method to make this a little more clear, I'll refer to the subject (the element with the directive) and the object (element the directive modifies). In this case the object is the only thing we need to store data on, since that is the element that toggleMinMax is going to change. The secondEle variable refers to a second subject, so in the case of data-max-num-cores-for-cluster-oakley: 28 the object is num-cores and the secondEle is cluster. So we assign a second event listener to cluster, but that listener can still only modify num-cores. So as long as you storeDefaults(num-cores) before those change handlers are added, I think it should all work out. If you tried it and it didn't work, or you have a specific example in mind that I am not considering, I'd definitely be open to discussing and getting to the bottom of it.

Copy link
Contributor

@Bubballoo3 Bubballoo3 Nov 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Oh I see what you are talking about now, and the easiest solution is to only collect the min/maxDefault one time for each element. So inside here you just need to check that wrapper.dataset['${dim}Default'] === undefined before wrapper.dataset['${dim}Default'] = n;

if (defaultVal !== null) {
const n = parseInt(defaultVal, 10);
if (!Number.isNaN(n)) {
wrapper.dataset[`${dim}Default`] = n;
}
}
});
}

/**
* Memorize element ID tokens and store default values
*
* @param {Array} elements
*/
function memorizeElements(elements) {
function initFormMetadata(elements) {
elements.each((_i, ele) => {
formTokens.push(mountainCaseWords(shortId(ele['id'])));
memorizeElement(ele);
storeDefaults(ele);
});
};
}

function makeChangeHandlers(prefix){

Expand All @@ -121,7 +150,7 @@ function makeChangeHandlers(prefix){
shortNameRex = new RegExp(`${idPrefix}_([\\w\\-]+)`);

const allElements = $(`[id^=${idPrefix}]`);
memorizeElements(allElements);
initFormMetadata(allElements);

allElements.each((_i, element) => {
if (element['type'] == "select-one"){
Expand Down Expand Up @@ -553,12 +582,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