wagtail
wagtail copied to clipboard
Fix max_num and display selection counter for MultipleChooserPanel
Fixes #11561
this PR may solve this issue in two way:
- make the selections input disabled when the user reach to maximum number.
- display counter for the choices of the user.
as shown below:
but when the user submit the forms become less than the maximum number by one so if max_num = 3 , and the user select all of them the forms only show two and if max_num = 5 , the forms only show 4 and so on. additionally this PR solve this issue also as shown below:
and also the user can select the maximum number in more than one time like this:
Manage this branch in Squash
Test this branch here: https://elhussienalmasrifix-max-num-of-lnqlu.squash.io
I have not read through this but looks like we could leverage the bulk-actions or counts controller here for better management of counting elements / changing behaviour based on counts and showing those values to the user.
welcome @lb- again , I will see the bulk-actions or counts controller as you suggest ,but can you please tell me all the defects of my code , and why bulk-actions instead this approach , this help me when I rewrite the code again. and also , I'm thinking to apply for gsoc with Wagtail ,but I don ,t make sure may apply or not, and the idea I choose to apply unfortunately removed so I choose other idea and this idea ,I am unfamiliar with and also I am unfamiliar in writing proposal ,so it will take me much time to write its proposal and also I am busy with other things , I just say that because I will return to this PR after completion its proposal after may be some week , if you have any idea how to write a good and fast proposal , please let me know ,Thanks.
Hey @elhussienalmasri - I've added a comment on the issue about a potentially different approach to this PR.
https://github.com/wagtail/wagtail/issues/11561#issuecomment-2002196259
Best to chat on Slack about your GSoC question.
I will continue working on this PR soon , but I have another PR , if and when you have time @lb- , please give feedback on this PR #11809 , Thanks.
@lb- While I understand the migration to stimulus is important, I feel like it shouldn't block fixing bugs in the current UI components. From a selfish perspective this is affecting my users, and preventing me from upgrading from inlinechoosers to multichooserpanels. Maybe the stimulus update is a good fast follow?
@dopry I have not really had a chance to look at this PR in full, my initial comment was more of a drive by.
I am not sure I will have a chance to review this PR in full for some time but have it on my radar, maybe others in the core team may have a look.
My original comment appears to be still valid though, there's a lot of manual handling of DOM selection and state tracking in this PR that is very prone to error. That could be handled by just adding a few data attributes (for the count tallying) and not too much additional code for the disabling of additional selections. My main concern is less about the migration to Stimulus but more about we have 70% of this functionality already in other code, and using that instead of re-writing it in the chooser modal code is worth looking into.
I guess if we do this PR we would need maybe want to bolster it a bit with some unit tests, in this area of code we do not have TypeScript to fall back on for simple errors either.
Maybe it's worth pinging @elhussienalmasri and seeing if you are still up for tackling this PR (or a fresh version of it).
What approach did you end up with @lb- ? if you are still determined to CountController (w-count) approach, then there are two issues:
- the submitted or previous forms count as I said, and find a solution to this issue, just by calculating it and attached it as an attribute to openModalButton (Not Recommended as @ gasman mentioned in my other PR), the second solution is to send it ( count of previous forms) to the server side. as I did in my other PR.
- we will need to modify w-count, or make another logic as it does not count the dynamic selection, the count of it is always zero in this case as I test it, (if the test is incorrect let me know)here:
wagtail/admin/templates/wagtailadmin/generic/chooser/chooser.html
<form action="{{ chosen_multiple_url }}" method="GET" data-multiple-choice-form data-controller="w-count" data-action="w-coun#count" data-w-count-find-value="input:checked" data-w-count-container-value="[data-multiple-choice-form]">
Even if change the data-w-count-find-value="input:disabled" ,change the attribute from checked to disable to know what I mean, you will notice the count does not change after selecting checkbox , it supposed after the selection checkbox the input with disabled attribute to be removed so the count must change and decrease by one but it remains the same, so w-count only count fixed elements as I test so it must be updated or make a new logic to count dynamic elements. I don,t have much experience with stimulus but I will try to update it if the approach is selected. and if you have any idea about this issue pleas let me know.
in addition to that the UI will not have a Counter. but in this PR, it will have.
but if we end up with the solution in this PR it will be Counter in the UI and yes I agree with you this code needs to be updated,
there's a lot of manual handling of DOM selection and state tracking in this PR that is very prone to error. That could be handled by just adding a few data attributes (for the count tallying) and not too much additional code for the disabling of additional selections.
and I think if it has any issue it will be easy to solve. and if this solution is selected and if you have any notes or feedback about this solution please let me and I will update it ASAP.
so please let me know what approach you end up with.