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.
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
MultipleChooserPanel UI does not respect max_num #11561
Comments
Related: #10754 (and to some extent #10496). These all stem from the fact that the chooser popup was designed to be a mechanism to get values into the panel, rather than being a representation of the panel's current state - however, that's evidently breaking user expectations, so accepting this as a valid bug. |
Hello, May I work on this issue? Can anyone help me with the basic code setup to reproduce the issue? |
@noobCoderVP You're welcome to give it a go, but I don't think this is going to be a straightforward fix. You can replicate this on the bakerydemo project - the Authors panel on BlogPage is a |
Ok, I will try to fix it. |
Hey @gasman, I was able to reproduce the issue on recipe page, When we select more than 3 authors, it selects the first 3 authors out of selected 4 authors. There is one more issue of duplicate authors, if we first select 1 author and again select the same author, name is duplicated in the authors list I have found the code where we are using MultipleChooserPanel I will try to fix both the issues, Let me know if there are any suggestions from your side |
@noobCoderVP The point about duplicates is covered in a separate issue: #10496. As mentioned there, there are situations where we do want to allow duplicates: for example, if you're building a slideshow of images with captions, and you want to allow the same image to appear multiple times (possibly with different captions). On that issue, I proposed that MultipleChooserPanel should only prevent selecting duplicates when a This is also very relevant to the current issue. In the case where we do allow duplicates, it doesn't make sense for the chooser to show existing choices when reopening it, because then there's no way to indicate "add another copy of this item". (It also makes the max_num logic really messy: suppose we have a slideshow with max_num=5, and it initially contains images A, B, C, A. The chooser should allow selecting one more item; however, if you de-select A within the chooser, it will now be valid to select 3 new ones.) Another consideration I didn't think about earlier: there's an open proposal (#9784) to change the behaviour of max_num in StreamField so that editors can exceed the maximum number of blocks while they are editing, but must bring it back under the limit before they save. If we go ahead with this change, it would be sensible to follow the same behaviour for InlinePanel / MultipleChooserPanel - in which case, the chooser should not enforce the maximum count, at least in the "allow duplicates" case. |
Ohh, functionality is pretty complex! Anyways let's first fix the enforcement of max_num in MultipleChooserPanel, I will fix it and test it. Let's handle duplicate case later. I am currently using docker to run bakerydemo project, I wanted to know how should I change the source code of wagtail locally to get it reflected in the bakerydemo project? |
I don't use Docker for Wagtail development myself, but https://github.com/wagtail/docker-wagtail-develop should allow you to set up a development environment. |
Hey @gasman, With your help finally I was able to setup the development environment, I have found the code that we need to change I wanted to understand more about the frontend code, Do you have any article where they have mentioned the organization of frontend code and how it is used? |
Hey @gasman I need a final help, I went through whole docs of Wagtail and saw how codebase works. I was able to understand the python side of the code with templates and models. I wanted to add a click event on checkbox selection which will prevent the selection of items more than the max_num. I wanted to know more about the code in client/src folder. What steps should I follow for that? |
I did say up-front it probably wouldn't be a straightforward fix! You might be able to get some further pointers in the #new-contributors channel on the Wagtail Slack. |
Yess, I have joined the channel. Will ask there. I knew this task is going to be tough but I enjoyed struggling and learnt a lot about the hardwork that you guyz have put to make Wagtail CMS. Model based admin panels is amazing!! I would have gone through whole codebase last 2-3 days. It's my first experience to understand such a huge codebase. @gasman you have helped a lot!! Thanks for that. I learnt a lot of things meanwhile. I will fix this issue at any cost and keep contributing... |
I'd like to propose some alternatives to the PR #11738 from @elhussienalmasri These ideas avoid baking in custom behaviour to the multiple chooser panel code but instead build on the other Stimulus controller code that already exists. At some point we will want to start tackling the migration of InlinePanel and MultipleChooserPanel to Stimulus so the less bespoke behaviour we have in that code the better. Additionally, making our generic Stimulus controllers more usable, robust and flexible is going to pay dividends down the road. Idea 1 - Use CountController (w-count)This might be the simplest approach, we can leverage this controller to conditionally show a warning/label when the count of selected checkboxes reaches some threshold. Either we show a warning when the count reaches the total (1a) or it exceeds it (1b). Another approach is that we show the counts of selection the entire time (1c). There three options should not require any new JavaScript code, just the additional html for the message and data attributes to use the controller. An example of how to get the controller connected to the form element. This will do the counting but you'll need to ensure the labels get set up. See the unit tests and other usage for examples.
It may be worth making it easier for this controller to count things in the controlled element only. Maybe when the container value is not set it could use the controlled element instead. Another option (1d) could be to allow for the labels to be updated when when the minValue has not been reached. Another option (1e) might be to enhance the controller to allow a target to be disabled if the threshold is reached. In this way we could dynamically disable the submit button. Although this is usually bad practice and instead we would want to consider a a better approach where it's enabled but clicking it does not submit but instead shows or focuses on a warning. Idea 2 - Use BulkController (w-bulk)This controller provides ways to manage groups of checkbox selections, allowing shift-click to select multiple in a row. Also allows a button to allow bulk selection of toggling all items. Finally it supports adding a class to the element once a selection has been made. It's doesn't, yet, support the ability to disable inputs based on some maximum number. Maybe, this controller could be enhanced to support this restriction and be used to solve this issue. We would need to add a Target support for the actual checkbox inputs and consider edge cases such as shift+click scenarios and toggling all. Note. We may end up wanting a combination of ideas to ensure we advise the user of their total selection compared to the max selection AND block the ability to go over the max selection. |
Great ideas @lb- , as I already raised PR , so I already have basic understand of the problem ,so I will continue work in this issue |
Thanks @elhussienalmasri - it's a good point about the counting of elements that may not be uniquely available in the DOM. See how you go with the suggestions above, I'll try to dig into this a bit more if I get the chance and update my comment above with a bit more code examples. But as @gasman pointed out earlier, looks like this may be a tricky one. |
yes I agree, this what make this issue tricky
plus we need to count the previous selected forms when user open the forms. |
Issue Summary
When using a MultipleChooserPanel with a max_num of 3, I am able to check more than 3 options. You can confirm the selection and max_num is respected, but it looks like an arbitrary items from the selection up to max_num are saved.
I would expect my existing selections to be highlight and not to be able to select more than my existing selection.
Steps to Reproduce
Technical details
Working on this
Anyone can contribute to this. View our contributing guidelines, add a comment to the issue once you’re ready to start.
The text was updated successfully, but these errors were encountered: