Skip to content

Ensure disabled state works as expected by changing links to buttons #48

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

Draft
joshmfrankel wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Ensure disabled state works as expected by changing links to buttons #48

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3dcad21
Ensure disabled state works as expected by changing links to buttons
joshmfrankel Apr 2, 2026
5f99371
Fix breaking specs
joshmfrankel Apr 2, 2026
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
127 changes: 90 additions & 37 deletions app/assets/javascripts/administrate_batch_actions/script.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,71 +1,124 @@
const init = function() {
var buttons = document.querySelectorAll("[data-batch-action-option='button']");
var checkboxes = document.querySelectorAll("[data-batch-action-option='checkbox']");
var selectAllCheckboxes = document.querySelector("[data-batch-action-option='select_all']");
const boundForms = new WeakSet();
const boundCheckboxes = new WeakSet();
const boundSelectAllCheckboxes = new WeakSet();

if (selectAllCheckboxes && checkboxes && buttons) {
const init = function () {
var buttons = document.querySelectorAll(
"[data-batch-action-option='button']",
);
const forms = document.querySelectorAll(
"form:has([data-batch-action-option='button'])",
);
var checkboxes = document.querySelectorAll(
"[data-batch-action-option='checkbox']",
);
var selectAllCheckboxes = document.querySelector(
"[data-batch-action-option='select_all']",
);

window.onpageshow = function(event) {
if (selectedItemIds()) {
checkboxes.forEach(function(checkbox) {
if (selectAllCheckboxes && checkboxes && buttons) {
window.onpageshow = function (event) {
if (selectedItemIds() && selectedItemIds().length === 0) {
checkboxes.forEach(function (checkbox) {
checkbox.checked = false;
});

selectAllCheckboxes.checked = false;
}
Comment on lines +20 to 27
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

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

window.onpageshow is currently clearing checkboxes only when selectedItemIds().length === 0. Since selectedItemIds() returns an array, this condition is the inverse of the previous behavior (it won’t clear when there are selected IDs, which is when BFCache/back-navigation tends to preserve checked boxes). Consider checking for > 0 (and re-running checkAndToggleActionButtons() after resetting) to keep button state consistent when navigating back.

Copilot uses AI. Check for mistakes.
};

selectAllCheckboxes.addEventListener('click', function(){
checkboxes.forEach(function(checkbox) {
checkbox.checked = selectAllCheckboxes.checked;
if (!isBoundElement(boundSelectAllCheckboxes, selectAllCheckboxes)) {
boundSelectAllCheckboxes.add(selectAllCheckboxes);

selectAllCheckboxes.addEventListener("click", function () {
checkboxes.forEach(function (checkbox) {
checkbox.checked = selectAllCheckboxes.checked;
});

checkAndToggleActionButtons();
});
}

checkAndToggleActionButtons();
});
forms.forEach(function (form) {
if (isBoundElement(boundForms, form)) return;
boundForms.add(form);

const confirmMessage =
form.querySelector("[data-confirm]")?.dataset?.confirm ||
"Are you sure you want to submit this form?";

buttons.forEach(function(button){
button.addEventListener('click', function(event){
button.href += '?' + selectedItemIds()
form.addEventListener("submit", function (event) {
event.preventDefault();

if (confirm(confirmMessage) === true) {
selectedItemIds().forEach(function (id) {
Comment on lines +46 to +54
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

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

confirmMessage falls back to a default string when no element in the form has data-confirm, which means every batch action submission will now show a confirmation dialog. If confirmation is intended to be opt-in (only when confirm:/data-confirm is provided), gate the confirm() call on the presence of data-confirm instead of defaulting to a message.

Copilot uses AI. Check for mistakes.
const hiddenInput = document.createElement("input");
hiddenInput.type = "hidden";
hiddenInput.name = "batch_action_ids[]";
hiddenInput.value = id;
form.appendChild(hiddenInput);
});
form.submit();
} else {
return;
}
Comment on lines +51 to +64
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

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

The submit handler always preventDefault() and then calls form.submit(). Programmatic form.submit() bypasses normal submit event processing (and may bypass Turbo/UJS interception), changing navigation/remote-submit behavior. Prefer letting the original submission proceed after injecting the hidden inputs (only preventDefault on cancel), or use a submission method that preserves the normal submit pipeline.

Suggested change
event.preventDefault();
if (confirm(confirmMessage) === true) {
selectedItemIds().forEach(function (id) {
const hiddenInput = document.createElement("input");
hiddenInput.type = "hidden";
hiddenInput.name = "batch_action_ids[]";
hiddenInput.value = id;
form.appendChild(hiddenInput);
});
form.submit();
} else {
return;
}
if (confirm(confirmMessage) !== true) {
event.preventDefault();
return;
}
form
.querySelectorAll("[data-batch-action-option='hidden_id']")
.forEach(function (hiddenInput) {
hiddenInput.remove();
});
selectedItemIds().forEach(function (id) {
const hiddenInput = document.createElement("input");
hiddenInput.type = "hidden";
hiddenInput.name = "batch_action_ids[]";
hiddenInput.value = id;
hiddenInput.dataset.batchActionOption = "hidden_id";
form.appendChild(hiddenInput);
});

Copilot uses AI. Check for mistakes.
});
});

checkboxes.forEach(function(checkbox){
checkbox.closest('td').addEventListener('click', function(event){
checkboxes.forEach(function (checkbox) {
if (isBoundElement(boundCheckboxes, checkbox)) {
return;
}
boundCheckboxes.add(checkbox);

checkbox.closest("td").addEventListener("click", function (event) {
event.stopImmediatePropagation();
})
});

checkbox.addEventListener('click', function(event) {
checkbox.addEventListener("click", function (event) {
event.stopImmediatePropagation();

checkAndToggleActionButtons();
})
})
});
});
}

function selectedItemIds() {
var ids = Array.prototype.filter.call(checkboxes, function(checkbox) {
if (checkbox.checked) { return checkbox }
}).map(function(checkbox) {
return 'batch_action_ids[]=' + checkbox.value
}).join('&');
var ids = Array.prototype.filter
.call(checkboxes, function (checkbox) {
if (checkbox.checked) {
return checkbox;
}
})
.map(function (checkbox) {
return checkbox.value;
});
return ids;
}

function checkAndToggleActionButtons() {
if (selectedItemIds()) {
buttons.forEach(function(button){
button.classList.remove('disabled');
button.removeAttribute('disabled');
if (selectedItemIds() && selectedItemIds().length > 0) {
buttons.forEach(function (button) {
button.disabled = false;
});
} else {
buttons.forEach(function(button){
button.classList.add('disabled');
button.setAttribute('disabled', 'disabled');
buttons.forEach(function (button) {
button.disabled = true;
});
}
}

function isBoundElement(boundElement, incomingElement) {
return boundElement.has(incomingElement);
}
};

document.addEventListener("DOMContentLoaded", function() { init() });
document.addEventListener("turbolinks:load", function() { init() });
document.addEventListener("turbo:load", function() { init() });
document.addEventListener("DOMContentLoaded", function () {
init();
});
document.addEventListener("turbolinks:load", function () {
init();
});
document.addEventListener("turbo:load", function () {
init();
});
8 changes: 7 additions & 1 deletion app/views/shared/administrate_batch_actions/_button.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,7 @@
<%= link_to name, path, class: "btn disabled #{html_options[:class]}", data: { batch_action_option: 'button', confirm: html_options[:confirm] }, method: :post %>
<%= button_to path,
class: "btn #{html_options[:class]}",
data: { batch_action_option: 'button', confirm: html_options[:confirm] },
method: :post,
Copy link
Copy Markdown
Contributor Author

@joshmfrankel joshmfrankel Apr 3, 2026

Choose a reason for hiding this comment

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

TODO: This should also set turbo_method: :post

disabled: true do %>
<%= name %>
<% end%>
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

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

Minor ERB formatting: <% end%> is missing a space before %>. Aligning with the rest of the codebase’s ERB style (e.g., <% end %>) improves readability and avoids inconsistent diffs.

Suggested change
<% end%>
<% end %>

Copilot uses AI. Check for mistakes.
2 changes: 1 addition & 1 deletion app/views/shared/administrate_batch_actions/_checkbox.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
<td>
<input type="checkbox" value="<%= value %>" name="batch_action_ids[]" data-batch-action-option="checkbox" />
<input type="checkbox" value="<%= value %>" data-batch-action-option="checkbox" />
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

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

There’s an extra space between attributes (value=..." data-batch...). This is harmless but adds noise to generated HTML and specs; consider normalizing spacing.

Suggested change
<input type="checkbox" value="<%= value %>" data-batch-action-option="checkbox" />
<input type="checkbox" value="<%= value %>" data-batch-action-option="checkbox" />

Copilot uses AI. Check for mistakes.
</td>
30 changes: 26 additions & 4 deletions spec/helpers_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,22 +2,44 @@

RSpec.describe 'Helpers', type: :helper do
it 'renders action button (default)' do
expect(helper.administrate_batch_actions_button('Delete All', '/')).to include '<a class="btn disabled button" data-batch-action-option="button" rel="nofollow" data-method="post" href="/">Delete All</a>'
expected_html = <<~HTML
<form class="button_to" method="post" action="/"><button class="btn button" data-batch-action-option="button" disabled="disabled" type="submit">
Delete All
</button></form>
HTML

expect(helper.administrate_batch_actions_button('Delete All', '/').squish).to include(expected_html.squish)
end

it 'renders action button (with class)' do
expect(helper.administrate_batch_actions_button('Delete All', '/', class: 'button text-danger')).to include '<a class="btn disabled button text-danger" data-batch-action-option="button" rel="nofollow" data-method="post" href="/">Delete All</a>'
expected_html = <<~HTML
<form class="button_to" method="post" action="/"><button class="btn button text-danger" data-batch-action-option="button" disabled="disabled" type="submit">
Delete All
</button></form>
HTML

expect(helper.administrate_batch_actions_button('Delete All', '/', class: 'button text-danger').squish).to include(expected_html.squish)
end

it 'renders action button (with confirm window)' do
expect(helper.administrate_batch_actions_button('Delete All', '/', class: 'button', confirm: 'Are you sure to do this?')).to include '<a class="btn disabled button" data-batch-action-option="button" data-confirm="Are you sure to do this?" rel="nofollow" data-method="post" href="/">Delete All</a>'
expected_html = <<~HTML
<form class="button_to" method="post" action="/"><button class="btn button" data-batch-action-option="button" data-confirm="Are you sure to do this?" disabled="disabled" type="submit">
Delete All
</button></form>
HTML

expect(helper.administrate_batch_actions_button('Delete All', '/', class: 'button', confirm: 'Are you sure to do this?').squish).to include(expected_html.squish)
end

it 'renders select all' do
expect(helper.administrate_batch_actions_select_all).to include '<input type="checkbox" data-batch-action-option="select_all" />'
end

it 'renders checkbox' do
expect(helper.administrate_batch_actions_checkbox(1)).to include '<input type="checkbox" value="1" name="batch_action_ids[]" data-batch-action-option="checkbox" />'
expected_html = <<~HTML
<input type="checkbox" value="1" data-batch-action-option="checkbox" />
HTML

expect(helper.administrate_batch_actions_checkbox(1).squish).to include(expected_html.squish)
end
end
Loading