I'm doing some facet processing in JavaScript and in the process I've made some minor improvements to the "List of Checkboxes" and "Dropdown" widgets.

* I fixed a markup bug with dropdown where the widget was a <select> inside of a <ul>
* I copied all the element data attributes from the list of links to the corresponding checkbox/option to make it more consistent to get the facet data
* I removed the links markup from the page instead of hiding them

CommentFileSizeAuthor
#35 interdiff.2810773.23.35.txt423 bytesduaelfr
#35 facets-improve_widget_js-2810773-35.patch6.64 KBduaelfr
#23 facets-improve_widget_js-2810773-23.patch6.64 KBrocketeerbkw
#21 facets-improve_widget_js-2810773-21.patch0 bytesrocketeerbkw
#17 facets-improve_widget_js-2810773-17.patch6.64 KBrocketeerbkw
#15 facets-improve_widget_js-2810773-15.patch6.65 KBrocketeerbkw
#13 interdiff.txt640 bytesborisson_
#13 improve_markup_and-2810773-13.patch6.53 KBborisson_
#10 facets-improve_widget_js-2810773-10.patch6.59 KBrocketeerbkw
#7 facets-improve_widget_js-2810773-7.patch6.75 KBrocketeerbkw
#2 facets-improve_widget_js-2810773-2.patch2.77 KBrocketeerbkw

Comments

rocketeerbkw created an issue. See original summary.

rocketeerbkw’s picture

rocketeerbkw commented
Status: Active » Needs review
StatusFileSize
new facets-improve_widget_js-2810773-2.patch2.77 KB

Status: Needs review » Needs work

The last submitted patch, 2: facets-improve_widget_js-2810773-2.patch, failed testing.

The last submitted patch, 2: facets-improve_widget_js-2810773-2.patch, failed testing.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented

Looks like the javascript tests we've written for those are not passing anymore, but the changes look good otherwise, do you have the time to look at the test fails?

rocketeerbkw’s picture

rocketeerbkw commented

Yes, I'll have time to fix tests.

rocketeerbkw’s picture

rocketeerbkw commented
Status: Needs work » Needs review
StatusFileSize
new facets-improve_widget_js-2810773-7.patch6.75 KB

I needed the facet values in JavaScript, but I had content with dashes, which broke splitting the facet id by dash. So I added two more data attributes for facet alias and facet value.

I fixed the tests for checkboxes. I started a test for dropdown, but got stuck on the last part, which fails. I'm not sure if $dropdown->selectOption() is incorrect, or if some kind of waiting needs to happen for the URL to change.

// Check that selecting one of the options changes the url.
// TODO: Fix this.
debug($options[1]->getValue()); // <-- /search-api-test-fulltext?f[0]=llama%3Aitem
$dropdown->selectOption($options[1]->getValue());
$current_url = $this->getSession()->getCurrentUrl();
debug($current_url); // <-- /search-api-test-fulltext
$this->assertTrue(strpos($current_url, 'search-api-test-fulltext?f%5B0%5D=llama%253Aitem') !== FALSE);

Status: Needs review » Needs work

The last submitted patch, 7: facets-improve_widget_js-2810773-7.patch, failed testing.

The last submitted patch, 7: facets-improve_widget_js-2810773-7.patch, failed testing.

rocketeerbkw’s picture

rocketeerbkw commented
Status: Needs work » Needs review
StatusFileSize
new facets-improve_widget_js-2810773-10.patch6.59 KB

Took awhile, but found out the dropdown test needs to wait for the page to load, unlike the checkbox test.

Status: Needs review » Needs work

The last submitted patch, 10: facets-improve_widget_js-2810773-10.patch, failed testing.

The last submitted patch, 10: facets-improve_widget_js-2810773-10.patch, failed testing.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs work » Needs review
StatusFileSize
new improve_markup_and-2810773-13.patch6.53 KB
new interdiff.txt640 bytes

Fails on php5, not on php7 - so I figure that's because php5 is slower. Increased the sleep time. Let's see if that helps.

Status: Needs review » Needs work

The last submitted patch, 13: improve_markup_and-2810773-13.patch, failed testing.

rocketeerbkw’s picture

rocketeerbkw commented
Status: Needs work » Needs review
StatusFileSize
new facets-improve_widget_js-2810773-15.patch6.65 KB

Not only is it PHP 5, but it's also sqlite, which I assume is slower?

I copied the wait format from testAddFacet and this works as expected locally (the wait condition fires for me way before the timeout). I even tried a wait of 5 minutes, and this single test passed in 42 seconds, hopefully the 6 seconds here is enough this time.

Status: Needs review » Needs work

The last submitted patch, 15: facets-improve_widget_js-2810773-15.patch, failed testing.

rocketeerbkw’s picture

rocketeerbkw commented
Status: Needs work » Needs review
StatusFileSize
new facets-improve_widget_js-2810773-17.patch6.64 KB

Setting timeout to 5 minutes, if this fails maybe it's actually broken?

Status: Needs review » Needs work

The last submitted patch, 17: facets-improve_widget_js-2810773-17.patch, failed testing.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented

I added an additional test for php5.5 + mysql and php7 + sqlite - to see if the problem is with php5 or with sqlite. Let's narrow this down.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented

The test also fails on php5 + mysql - so I'd assume something is really broken on the php side - probably in the test-code - as that's the only phpcode you've changed. I'll investigate after work.

rocketeerbkw’s picture

rocketeerbkw commented
Status: Needs work » Needs review
StatusFileSize
new facets-improve_widget_js-2810773-21.patch0 bytes

I was able to get test failures on a local php 5.6 with mysql and sqlite.

It looks like the ordering of the options is different? When I was selecting the nth option, that meant two different facets between the versions of PHP, and so two different URLs were being loaded. I haven't looked into how/why the ordering is different.

Poor mans interdiff:

- $dropdown->selectOption($options[1]->getValue());
+ $dropdown->selectOption('item (3)');

Status: Needs review » Needs work

The last submitted patch, 21: facets-improve_widget_js-2810773-21.patch, failed testing.

rocketeerbkw’s picture

rocketeerbkw commented
Status: Needs work » Needs review
StatusFileSize
new facets-improve_widget_js-2810773-23.patch6.64 KB

Status: Needs review » Needs work

The last submitted patch, 23: facets-improve_widget_js-2810773-23.patch, failed testing.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented

It looks like the ordering of the options is different? When I was selecting the nth option, that meant two different facets between the versions of PHP, and so two different URLs were being loaded. I haven't looked into how/why the ordering is different.

Sorting in php5 vs php7 is different - that's probably the reason why.

The remaining testfail on php5 is because of #2820575: Fix rest tests. We still have to figure that out.

In the meantime - this is rtbc to me.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs work » Needs review

Back to NR now that #2820575: Fix rest tests has been committed.

The last submitted patch, 21: facets-improve_widget_js-2810773-21.patch, failed testing.

rocketeerbkw’s picture

rocketeerbkw commented
borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Assigned: Unassigned » strykaizer
Status: Needs review » Reviewed & tested by the community

Looks good to me. Assigning this to @StryKaizer for review

mpp’s picture

mpp commented

For regular links it seems ok but for checkboxes, the data-drupal-facet-item-value attribute seems to be missing.

Shouldn't we add the facet data (facet id+raw_value) to the render array as well?

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Reviewed & tested by the community » Needs work

Needs work based on @mpp's tests.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented

Actually, not sure about the changes requested by @mpp. We discussed this during the monthly hangout and agreed that this is a good idea to commit. However I'd like to commit a couple of other issues first so holding off on committing today to see if the patch still applies after those were committed.

mpp’s picture

mpp commented

@borisson_ that's fine for me

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Issue tags: +Needs reroll

The patch no longer applies and I think this conflicts with #2725383: Use drupalSettings instead of data-attribute to pass config to js.

duaelfr’s picture

duaelfr
he/him
Primary language French
Location Montpellier, France
commented
Assigned: strykaizer » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new facets-improve_widget_js-2810773-35.patch6.64 KB
new interdiff.2810773.23.35.txt423 bytes

I rerolled the patch and made a minor improvement to dropdown JS (see interdiff for this change).

  • borisson_ committed 98242c0 on 8.x-1.x authored by rocketeerbkw 
    Issue #2810773 by rocketeerbkw, DuaelFr: Improve markup and consistency...
borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Fixed

Thanks for the reroll @DuaelFr and for all the hard work on this @rocketeerbkw. Sorry for taking so long before this finally got committed.

Status: Fixed » Closed (fixed)

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