Support from Acquia helps fund testing for Drupal Acquia logo

Comments

StryKaizer created an issue. See original summary.

borisson_’s picture

Issue summary: View changes
stijn26’s picture

Status: Active » Needs review
FileSize
7.6 KB
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

Status: Needs work » Needs review
FileSize
8.77 KB
16.37 KB

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

Status: Needs work » Needs review
FileSize
907 bytes
17.25 KB

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

FileSize
3.36 KB
21.46 KB

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

Status: Needs work » Needs review
FileSize
991 bytes
21.35 KB
borisson_’s picture

FileSize
19.84 KB

Reverted #11.

borisson_’s picture

FileSize
19.72 KB
2.61 KB

Ran eslint over the new js file.

StryKaizer’s picture

FileSize
4.4 KB
20.99 KB

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

Status: Needs work » Needs review
FileSize
1.32 KB
21.1 KB
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

FileSize
2.73 KB

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.