Support from Acquia helps fund testing for Drupal Acquia logo

Comments

michiellucas created an issue. See original summary.

borisson_’s picture

Version: » 8.x-1.x-dev

With the new CheckboxWidget in, work on this should be easier.

Leksat’s picture

Assigned: Unassigned » Leksat

Working on this.

Leksat’s picture

Assigned: Leksat » Unassigned
Status: Active » Needs review
FileSize
4.42 KB

Here is a working dropdown.

Some notes:
1. I used URLs as the select option values. That will make possible to use JS to go to URL directly rather than submit form. (I saw this in D7 and found it pretty useful)
2. Dropdown only works well with the show_only_one_result mode enabled. Probably we also need form validation for this.
3. I'm not sure if I will have time to work on this issue or on #2625188: Allow limiting to one active item.

borisson_’s picture

Assigned: Unassigned » borisson_
Status: Needs review » Needs work
Issue tags: +Needs tests

This looks great, I'll write a unit test and integration test for this as well so we can get this in.

borisson_’s picture

Status: Needs work » Needs review
FileSize
11.43 KB
15.05 KB

I have done some work on this, but I don't have enough time now to finish this. I'll come back to this issue later today or tomorrow. Anyway - saving work.

Status: Needs review » Needs work

The last submitted patch, 6: widget_dropdown_select-2636328-6.patch, failed testing.

The last submitted patch, 6: widget_dropdown_select-2636328-6.patch, failed testing.

borisson_’s picture

Assigned: borisson_ » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
7.32 KB
14.63 KB

Tests should be green now.

Leksat’s picture

Interesting, checkbox widget uses form build info to inject and retrieve the facet, but here it works from a constructor. Probably this can be removed?

borisson_’s picture

I think this way is a little bit cleaner and we should create a followup to make the checkbox look like this one, would you agree?

Leksat’s picture

FileSize
14.67 KB
634 bytes

Found a small bug: user may be redirected to /_none path.

Leksat’s picture

borisson_’s picture

Status: Needs review » Fixed

The code here looks great and I think we can commit this and there's a bunch of test coverage added for this. If there are problems with this patch we'll get issues filed for them so we can add specific test coverage for that as well.

Thank you very much for your work on this!

  • borisson_ committed e2d4767 on 8.x-1.x authored by Leksat
    Issue #2636328 by borisson_, Leksat: Widget dropdown - select
    

Status: Fixed » Closed (fixed)

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