Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
The CheckboxWidget is currently a copy/paste of the LinksWidget with an addition of a basic configuration form. The rendering of this widget however is exactly the same as the LinkWidget. This is wrong.
In this issue, you should change the rendering of the widget to make sure that is uses radio or checkboxes (depending on the chosen setting in the form).
Comment | File | Size | Author |
---|---|---|---|
#17 | widget_checkboxwidget-2596337-17.patch | 12.95 KB | borisson_ |
Comments
Comment #2
borisson_If #2608228: Add tests for widgets lands before this one does, the test needs to be updated as well.
Comment #3
michiellucas CreditAttribution: michiellucas as a volunteer commentedFirst version of the checkbox widget, still needs work to have clickable checkboxes.
Comment #4
borisson_I spent some time working on this over the weekend, can't get it to work yet but this should be the way to go, I think.
Tests also pass again. Still needs more work.
Comment #5
michiellucas CreditAttribution: michiellucas as a volunteer commentedI was just working on this issue, see my solution in attachment. Going to test your patch
I created a JS library to handle the onclick event of the checkbox
Comment #6
borisson_@michiellucas: the solution you created doesn't have real checkboxes, these are just links that also have a checkbox shown next to them.
Even if we'd go for that solution, creating a link with a
<a href=''"></a>
concatenation in code is not the preferred way. The preferred way is to use the\Drupal\Core\Link
value object.I think we should go for a real form, because of #2625188: Allow limiting to one active item, when that patch lands, we have to add in a check for
facet->getShowOnlyOneResult()
and if that returns true, we should show radios instead of checkboxes.Comment #7
michiellucas CreditAttribution: michiellucas as a volunteer commentedDo you want to store the $url in the values of the checkbox and handle the redirect in a form_submit?
I agree with the approach to the Link, was a quick idea that i overlooked when submitting patch;
Making a form out of this facet is maybe a bit to difficult for what it is? Integrating Issue #2625188 should not be such a problem. We should only print out the ones that are marked as active $result->isActive()
Your choice how to proceed, thanks for the feedback.
Michiel
Comment #8
borisson_We discussed this in irc just now. This is the relevant log of that converation:
So this means that we should have a solution that works without JS (and a POST form) and a have some JS available to enhance the form (and hide the submit button (add the
js-hide
class)).Comment #9
borisson_I discussed this with Wim Leers today, we think this is the correct way to go currently the facet object isn't passed along to the buildForm method.
We agree that this feels like a more solid solution though. Uploading intermediate work.
Comment #11
borisson_Spending more time with this.
Comment #12
borisson_This is going in the right direction, tests still break because I didn't look at them yet.
This correctly renders now and that widget looks great.
There's no redirect yet though, so taking a look at that after lunch.
Comment #14
borisson_Fixes the unit test + cleanup of the checkbox widget.
Comment #15
borisson_This works now, locally. We need to add an integration test for this still, starting on that now.
Comment #16
borisson_This adds a new
WidgetIntegrationTest
that adds an integration test for this as well. Would love to get a review on this.Comment #17
borisson_We discussed this issue at the hangout and as a result of that discussion I added some extra comments in the submitForm.
Comment #18
Nick_vhUpdated comments look great - good to go for me if tests pass :)
Comment #19
michiellucas CreditAttribution: michiellucas as a volunteer commentedGoing to update our facets module, looking forward to test this patch !
Comment #20
borisson_@michiellucas: I was just about to commit this, if you're going to test this, do you want me to wait with committing this so we can get feedback or will a new issue suffice?
Comment #21
michiellucas CreditAttribution: michiellucas as a volunteer commentedDon't wait, tested it really quick but at this moment i am working on another module so i will start new issue if i find issue.
great work!
Comment #22
borisson_Thanks for helping us test this and for the patches you provided in this issue.
Comment #30
borisson_