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).

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

borisson_ created an issue. See original summary.

borisson_’s picture

If #2608228: Add tests for widgets lands before this one does, the test needs to be updated as well.

michiellucas’s picture

First version of the checkbox widget, still needs work to have clickable checkboxes.

borisson_’s picture

Status: Active » Needs work
FileSize
4.28 KB
4.95 KB

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.

michiellucas’s picture

I 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

borisson_’s picture

@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.

michiellucas’s picture

Do 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

borisson_’s picture

We discussed this in irc just now. This is the relevant log of that converation:

Nick_vh: borisson_: what is your argument to use a real form?
borisson_: Nick_vh: I was going to say accessibility
borisson_: Nick_vh: but I think that won't be a problem since it's actually links.
Nick_vh: imho, we should probably avoid any form that isn’t really necessary (without losing accessibility) - the problem is that you then submit it to Drupal and you have to do a redirect right after, causing the page to load twice for no real reason?
borisson_: Nick_vh: you create a GET-form ?
borisson_: but a GET form wouldn't work for pretty paths
Nick_vh: hm, true - that reasoning is only valid with new paths and the keyword form then
borisson_: I don't understand why you'd want to create a form element, like in the proposed patch, and then react trough the links anyway.
Nick_vh: well, if it works with a Get FORM - why not use the right tools for the job I guess
Nick_vh: borisson_: real life usecase of checkbox search
Nick_vh: http://www.vandenborre.be/wasmachine-droogkast/wasautomaat
Nick_vh: they even use a POST form
Nick_vh: with a whole bunch of javascript hackery
borisson_: Nick_vh: the vandenborre site also uses javscript
borisson_: facets don't work without JS.
Nick_vh: borisson_: I just noticed that ;-)
StryKaizer: we could do both? or is that overkill? :)
StryKaizer: js and form with drupal_goto
StryKaizer: for non-js fallback
Nick_vh: imho that’s not overkill and probably the way to go
Nick_vh: auto-submit the form with JS if JS is available
StryKaizer: core is supporting both, so we prolly should too
borisson_: StryKaizer: agreed.
Nick_vh: and have a button with “activate filter” or so
Nick_vh: so that one can select multiple checkboxes in non JS context to submit it
Nick_vh: AND it becomes testable :)
StryKaizer: but dont use GET form, use custom js, so pretty paths works too ;)
Nick_vh: so then it does become a POST form which is basically “made unclear” through the JS
Nick_vh: ?
StryKaizer: Yup
borisson_: POST forms are uncacheable though; currently
Nick_vh: how is the custom JS then using pretty paths or any of those processors for that matter?
StryKaizer: its only a post in non-js though
StryKaizer: can we check what facetapi in d7 did?
StryKaizer: lemme see
StryKaizer: ow right
StryKaizer: they use fake checkboxes, which are actually links
StryKaizer: cant we just do that?
Nick_vh: probably need to do a pro and contra there
Nick_vh: how does that affect accessibility
StryKaizer: No need to do js and forms when we just create a link and add a checkbox/radio inside the link
Nick_vh: can’t find direct references that this is forbidden
borisson_: that sounds like a good compromise, for a11y as well.
Nick_vh: so having just a checkbox input type without any form submit is ok, the problem is when the checkbox is checked and there is no JS
Nick_vh: what are you doing then?
StryKaizer: eh?
StryKaizer: we always render links
StryKaizer: so I dont get the JS part, we shouldnt use js?
Nick_vh: if there is no JS
borisson_: yes, but links with checkboxes next to them? what happens when you click the checkbox?
Nick_vh: and you show a checkbox without button
borisson_: or is the checkbox an image + css?
StryKaizer: http://tuinrama.be/shop
Nick_vh: indeed, nothing happens with the checkbox
StryKaizer: facetapi d7
Nick_vh: borisson_: I don’t think we can replace checkboxes with images :)
StryKaizer: I dont get the question :)
borisson_: Nick_vh: those checkboxes are images.
StryKaizer: Oh
StryKaizer: well, we can just render instead, no?  (Not sure if thats the way to go)
Nick_vh: borisson_: but can we enforce using images in facetapi?
Nick_vh: highly doubtful of that
StryKaizer: I prolly should check a vanilla facetapi :)
Nick_vh: StryKaizer: even then, how are you going to take action in non-JS world on a freshly checked checkbox
StryKaizer: Nick_vh : you are right
Nick_vh: given the fact that you don’t control the styling
StryKaizer: Mondaymorning made me think that would trigger the link too
Nick_vh: it’s a pitty as there are very pretty solutions out there
StryKaizer: Working on my caffeine levelborisson_: Okay, so we agree that having checkboxes with no form is a suboptimal solution?
Nick_vh: borisson_: my recommendation - a real form that works without JS, and with JS it is prettified so that you can click on the word and it directly posts the result using the GET form. Not fully sure how you can do the pretty paths pre-processing for telling it what URL it becomes without making them all separate forms though
Nick_vh: but perhaps my form-fu knowledge is suboptimal lately ;-)
borisson_: So my only problem with that solution is that we'd be creating a form per facet block.
Nick_vh: and why is that a problem?
borisson_: not a technical problem, i think it's weird UX to have 5 submit buttons in a sidebar.
Nick_vh: borisson_: that’s pure for testing purposes and for those weirdo’s that do not have JS
Nick_vh: but it does follow the Drupal convention in the sense that it needs to work without JS
StryKaizer: borisson_ : thats no issue indeed
StryKaizer: you even SHOULD have those buttons
StryKaizer: coolblue has them too, when no JS is available

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)).

borisson_’s picture

Version: » 8.x-1.x-dev
Status: Needs work » Needs review
FileSize
4.63 KB

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.

Status: Needs review » Needs work

The last submitted patch, 9: widget_checkboxwidget-2596337-9.patch, failed testing.

borisson_’s picture

Assigned: Unassigned » borisson_
Issue tags: -Novice

Spending more time with this.

borisson_’s picture

Status: Needs work » Needs review
FileSize
5.21 KB
4.82 KB

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.

Status: Needs review » Needs work

The last submitted patch, 12: widget_checkboxwidget-2596337-12.patch, failed testing.

borisson_’s picture

Status: Needs work » Needs review
FileSize
7.39 KB
10.06 KB

Fixes the unit test + cleanup of the checkbox widget.

borisson_’s picture

FileSize
1.2 KB
10.11 KB

This works now, locally. We need to add an integration test for this still, starting on that now.

borisson_’s picture

Assigned: borisson_ » Unassigned
FileSize
2.92 KB
12.52 KB

This adds a new WidgetIntegrationTest that adds an integration test for this as well. Would love to get a review on this.

borisson_’s picture

FileSize
1.73 KB
12.95 KB

We discussed this issue at the hangout and as a result of that discussion I added some extra comments in the submitForm.

Nick_vh’s picture

Updated comments look great - good to go for me if tests pass :)

michiellucas’s picture

Going to update our facets module, looking forward to test this patch !

borisson_’s picture

@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?

michiellucas’s picture

Don'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!

borisson_’s picture

Status: Needs review » Fixed

Thanks for helping us test this and for the patches you provided in this issue.

  • borisson_ committed 3875fa2 on 8.x-1.x
    Issue #2596337 by borisson_, michiellucas: Widget: CheckboxWidget
    

The last submitted patch, 9: widget_checkboxwidget-2596337-9.patch, failed testing.

The last submitted patch, 12: widget_checkboxwidget-2596337-12.patch, failed testing.

The last submitted patch, 14: widget_checkboxwidget-2596337-14.patch, failed testing.

The last submitted patch, 15: widget_checkboxwidget-2596337-15.patch, failed testing.

The last submitted patch, 16: widget_checkboxwidget-2596337-16.patch, failed testing.

Status: Fixed » Needs work

The last submitted patch, 17: widget_checkboxwidget-2596337-17.patch, failed testing.

borisson_’s picture

Status: Needs work » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.