Fixed #36629 -- Added "Choose all" and "Remove all" button in the adm… #19942
Conversation
…in m2m widget filter_vertical The "Choose all" and "Remove all" button was missing in the admin many to many widget filter_vertical. Added it with one line change in the css.
|
Hello! Thank you for your contribution 💪 As it's your first contribution be sure to check out the patch review checklist. If you're fixing a ticket from Trac make sure to set the "Has patch" flag and include a link to this PR in the ticket! If you have any design or process questions then you can ask in the Django forum. Welcome aboard ⛵️! |
There was a problem hiding this comment.
Thank you for work on this ticket @miftahulkabir ⭐
It seems that some margin is needed between the “Choose all” / “Remove all” buttons and the select element in vertical mode.
index fbf0d88b40..8e6f729d41 100644
--- a/django/contrib/admin/static/admin/css/responsive.css
+++ b/django/contrib/admin/static/admin/css/responsive.css
@@ -287,10 +287,6 @@ input[type="submit"], button {
flex: 0 1 auto;
}
- .stacked select {
- margin-bottom: 0;
- }
-
.stacked .selector-available, .stacked .selector-chosen {
width: auto;
}The disabled check wasn’t performed in vertical mode because the buttons weren’t provided.
Now that the “Choose all” and “Remove all” buttons are available in both vertical and horizontal modes, we should update the tests to cover both.
index 7588c2cc32..3985d50283 100644
--- a/tests/admin_widgets/tests.py
+++ b/tests/admin_widgets/tests.py
@@ -1297,13 +1297,12 @@ class HorizontalVerticalFilterSeleniumTests(AdminWidgetSeleniumTestCase):
remove_all_button = "#id_%s_remove_all" % field_name
self.assertEqual(self.is_disabled(choose_button), choose_btn_disabled)
self.assertEqual(self.is_disabled(remove_button), remove_btn_disabled)
- if mode == "horizontal":
- self.assertEqual(
- self.is_disabled(choose_all_button), choose_all_btn_disabled
- )
- self.assertEqual(
- self.is_disabled(remove_all_button), remove_all_btn_disabled
- )
+ self.assertEqual(
+ self.is_disabled(choose_all_button), choose_all_btn_disabled
+ )
+ self.assertEqual(
+ self.is_disabled(remove_all_button), remove_all_btn_disabled
+ ) if mode == "horizontal":
self.selenium.find_element(By.ID, choose_all_button).click()
elif mode == "vertical":
# There 's no 'Choose all' button in vertical mode, so individually
# select all options and click 'Choose'.
for option in self.selenium.find_elements(
By.CSS_SELECTOR, from_box + " > option"
):
option.click()
self.selenium.find_element(By.ID, choose_button).click()I think this part doesn't need to be separated between vertical and horizontal modes either.
like this...
index 7588c2cc32..3985d50283 100644
--- a/tests/admin_widgets/tests.py
+++ b/tests/admin_widgets/tests.py
@@ -1297,13 +1297,12 @@ class @@ -1374,16 +1364,7 @@ class HorizontalVerticalFilterSeleniumTests(AdminWidgetSeleniumTestCase):
)
# Click 'Remove all' --------------------------------------------------
- if mode == "horizontal":
- self.selenium.find_element(By.ID, remove_all_button).click()
- elif mode == "vertical":
- # There 's no 'Remove all' button in vertical mode, so individually
- # select all options and click 'Remove'.
- for option in self.selenium.find_elements(
- By.CSS_SELECTOR, to_box + " > option"
- ):
- option.click()
- self.selenium.find_element(By.ID, remove_button).click()
+ self.selenium.find_element(By.ID, remove_all_button).click()
self.assertSelectOptions(
from_box,
[
| .stacked .selector-chooseall, .stacked .selector-clearall { | ||
| display: none; | ||
| display: block; | ||
| } |
There was a problem hiding this comment.
How about removing it entirely? Since the default display value is block :)
|
@Antoliny0919 Hi SiHyunLee, thanks a lot for showing and pointing out the all the changes I need to do in the code. This is my first PR and I am very new to django codebase. The way you reviewed this thing will help me a lot. I will finish the changes and push the changes to this PR. Thanks again. |
…vertical and horizontal modes. Removed the display:block section as well.
|
Hi @Antoliny0919, I pushed the changes to this PR. |
There was a problem hiding this comment.
Thank you for the update @miftahulkabir 😁
I’ve turned the Needs Tests flag on for the ticket.
Once your work is complete, please turn the Needs Tests flag off.
This document might be helpful:
https://docs.djangoproject.com/en/5.2/internals/contributing/triaging-tickets/#needs-documentation
For reference, a ticket will be added to the review queue only when Needs Tests, Needs Documentation, and Patch needs Improvement are all turned off.
Once it’s in the review queue, a fellows(maintainer) can review it.
(However, it’s uncertain exactly when the review will happen, so some waiting is necessary!)
| /* .stacked .selector-chooseall, .stacked .selector-clearall { | ||
| display: block; | ||
| } */ |
There was a problem hiding this comment.
Is there a reason why this part was commented out?
From my perspective, it seems like removing this CSS selector wouldn’t cause any issues.
| if mode == "horizontal": | ||
| self.selenium.find_element(By.ID, remove_all_button).click() | ||
| elif mode == "vertical": |
There was a problem hiding this comment.
It seems like the same pattern exists in one more place.
https://github.com/django/django/blob/main/tests/admin_widgets/tests.py#L1342
|
It looks like the PR title got truncated! I’d appreciate it if you could fix it. |
|
Superseded by #20396 |
…in m2m widget filter_vertical
The "Choose all" and "Remove all" buttons were missing in the admin ManyToMany filter_vertical widget. Fixed the issue by updating the CSS to display these buttons.
Trac ticket number
ticket-36629
Branch description
Added the missing "Choose all" and "Remove all" buttons to the ManyToMany
filter_verticalwidget in the Django admin by updating the CSS.Checklist
mainbranch.