Comments

StryKaizer created an issue. See original summary.

borisson_’s picture

Issue summary: View changes
stijn26’s picture

stijn26’s picture

To-do: ensure that there is only one active item. Configurable placeholder text?

Status: Needs review » Needs work

The last submitted patch, 3: use_get_request_for-2713875-3.patch, failed testing.

The last submitted patch, 3: use_get_request_for-2713875-3.patch, failed testing.

borisson_’s picture

Thanks for that patch Stijn!

We already have a processor that ensures only 1 item can be active, we should probably make sure that processor is enabled by default for this widget. I attached a patch that removes a useless unit test and updates a failing one. The integration test'll still fail.

I think the easiest way forward here is implementing #2611198: Give widgets the ability to require settings and it's inverse.

Status: Needs review » Needs work

The last submitted patch, 7: use_get_request_for-2713875-7.patch, failed testing.

The last submitted patch, 7: use_get_request_for-2713875-7.patch, failed testing.

borisson_’s picture

This makes the integration test pass, we could commit this, but I wouldn't feel very confident right now, without any test coverage. So I'd love to get #2719065: Javascript test for widget in first, so we can test the JS that makes this into a dropdown.

So, let's keep this patch as-is for now, and we'll come back here soon.

borisson_’s picture

I broke the js, I think. But I don't know why.

Status: Needs review » Needs work

The last submitted patch, 11: use_get_request_for-2713875-11.patch, failed testing.

The last submitted patch, 11: use_get_request_for-2713875-11.patch, failed testing.

borisson_’s picture

borisson_’s picture

StryKaizer’s picture

Should be working now.
Since we can not access facet configuration from a widget plugin at this moment (this is the only plugin which requires this), I added a visible warning instead notifying the end user the "Make sure only one result can be shown." configuration is required to behave as a standard dropdown.

Status: Needs review » Needs work

The last submitted patch, 17: use_get_request_for-2713875-17.patch, failed testing.

The last submitted patch, 17: use_get_request_for-2713875-17.patch, failed testing.

StryKaizer’s picture

borisson_’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
3.5 KB
21.16 KB

Eslint + phpcs fixes, changed 1 doc comment in js. RTBC'd the patch because I think it looks great. We can add test coverage next in #2719065: Javascript test for widget

claudiu.cristea’s picture

Nits.

  1. +++ b/facets.libraries.yml
    @@ -28,3 +28,13 @@ drupal.facets.checkbox-widget:
    +    - core/jquery.once
    \ No newline at end of file
    

    Needs a newline at the end of file.

  2. +++ b/tests/src/Unit/Plugin/widget/DropdownWidgetTest.php
    @@ -0,0 +1,111 @@
    +  private function buildLinkAssertion($text, $count = 0, $active = FALSE, $show_numbers = TRUE) {
    

    Why is this private function? Probably protected function?

borisson_’s picture

Status: Reviewed & tested by the community » Fixed

Committed, see you in #2719065: Javascript test for widget to add test coverage.

  • borisson_ committed 77ea675 on 8.x-1.x
    Issue #2713875 by borisson_, StryKaizer, stijn26: Use GET request for...
borisson_’s picture

Oh, sorry about #22, didn't see that before I committed, I've fixed those in the attached patch, you agree with those changes?

claudiu.cristea’s picture

Status: Fixed » Reviewed & tested by the community

  • borisson_ committed 2630010 on 8.x-1.x
    Issue #2713875 by borisson_, claudiu.cristea: Followup, change method...
borisson_’s picture

Status: Reviewed & tested by the community » Fixed

Committed that followup patch.

Status: Fixed » Closed (fixed)

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