Replace jumpMenu controls with menu-button components

[demiankatz]

This PR supersedes #2492.

TODO

• Convert card selector in librarycards/selectcard.phtml
• Convert sort control in myresearch/controls/sort.phtml
• Convert search scheduler in myresearch/schedulesearch.phtml
• Convert limit control in search/controls/limit.phtml
• Convert sort control in search/controls/sort.phtml
• Convert search scheduler in search/history-table.phtml
• Make sure behavior remains consistent with (and passes tests from) Fix result limit and sort templates. #3031.
• Adjust styles for appropriate look and feel
• Eliminate obsolete jumpMenu Javascript once all related code is refactored
• Update changelog to indicate removal of jumpMenu behavior
• Ensure appropriate keyboard, focus and accessibility/screen reader behavior in menu-button component.
• Run full test suite before merging

[demiankatz]

Okay, I hope I've gotten the ball rolling here. Notes on my progress so far:

1.) I've added a new showSelectedValue setting to the menu-button control, which can be used to include the current selected value in the button, so you don't have to open the menu to see what is selected. I'm sure this could be styled more attractively, but consider this an initial proof-of-concept.

2.) I've converted the two jumpMenu standard search controls (sort/limit) to use menu-button instead. If you turn on limits in searches.ini, you'll see that currently the controls stack on top of each other instead of displaying side-by-side. I'm sure we can fix this by applying appropriate classes or other style changes. Again, this is a proof of concept for now and needs more polish.

If @crhallberg and/or @stephanieleary can take this further while I'm on vacation next week, I would be grateful for any assistance!

[demiankatz]

@crhallberg / @stephanieleary, I had more time to work on this today, so I finished up all the template conversion and Javascript cleanup. In the process, I added a couple additional new features to the menu-button component -- you can now set arbitrary link attributes (needed for data elements in order to clear the MyResearch AJAX cache), and you can omit the toggleLabel (in case you JUST want to display the value -- useful for the scheduled search controls).

The only remaining work is to adjust the styles (since some things still don't look quite right -- particularly the layout of sort/limit controls in search results) and then fix any broken tests. I haven't put any time into tests yet since I expect that some details may change as a result of styling. @crhallberg, can you please try to take a look at styles this week so we can wrap this up?

[demiankatz]

For future reference, here are the test failures that will need to be addressed once style is finalized:

There were 5 failures:

1) VuFindTest\Mink\SavedSearchesTest::testNotificationSettings
Failed asserting that actual size 0 matches expected size 2.

/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/SavedSearchesTest.php:326
/usr/local/vufind/module/VuFind/src/VuFindTest/Feature/AutoRetryTrait.php:100

2) VuFindTest\Mink\SavedSearchesTest::testNotificationsInSearchToolbar
Element not found: select[name="schedule"] index 0
Failed asserting that null is of type "object".

/usr/local/vufind/module/VuFind/src/VuFindTest/Integration/MinkTestCase.php:387
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/SavedSearchesTest.php:385
/usr/local/vufind/module/VuFind/src/VuFindTest/Feature/AutoRetryTrait.php:100

3) VuFindTest\Mink\SearchLimitTest::testCustomLimits
Element not found: #limit index 0
Failed asserting that null is of type "object".

/usr/local/vufind/module/VuFind/src/VuFindTest/Integration/MinkTestCase.php:387
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/SearchLimitTest.php:100
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/SearchLimitTest.php:147
/usr/local/vufind/module/VuFind/src/VuFindTest/Feature/AutoRetryTrait.php:100

4) VuFindTest\Mink\SearchLimitTest::testInvalidLimits
Element not found: #limit index 0
Failed asserting that null is of type "object".

/usr/local/vufind/module/VuFind/src/VuFindTest/Integration/MinkTestCase.php:387
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/SearchLimitTest.php:100
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/SearchLimitTest.php:167
/usr/local/vufind/module/VuFind/src/VuFindTest/Feature/AutoRetryTrait.php:100

5) VuFindTest\Mink\SearchLimitTest::testNonNumericLimitValues
Element not found: #limit index 0
Failed asserting that null is of type "object".

/usr/local/vufind/module/VuFind/src/VuFindTest/Integration/MinkTestCase.php:387
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/SearchLimitTest.php:100
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/SearchLimitTest.php:187
/usr/local/vufind/module/VuFind/src/VuFindTest/Feature/AutoRetryTrait.php:100

FAILURES!
Tests: 2314, Assertions: 11810, Failures: 5, Skipped: 2.

[EreMaijala]

I took a quick look, and while the components work, there are some issues remaining, some of them obviously down to one's preferences:

1. The current version is very bland and doesn’t stand out as a control.
2. The label should maybe stand out a bit more as well (not sure I like it blending in with the current value).
3. Takes a lot of space vertically at least when compared to select controls. See the screenshots below for comparison.
4. The dropdown menu doesn’t align properly for the right-aligned search controls.
5. The heavy padding especially on left makes it look like the control is indented too much.
6. Should maybe drop up when at the bottom of viewport or page.

Old:

New:

[demiankatz]

Thanks, @EreMaijala! I'm hoping that @crhallberg can help with these improvements. Some general notes:

1.) The menu-button component was originally designed for the drop-downs in the VuFind header, and I think the current styles reflect that. It might be a good idea to give the component different default styles, and then override those styles for its appearance in the header.

2.) When I implemented the "show current value" feature, I concatenated the value into the control label because it was the fastest way to finish implementing the functionality, not because I think that's the way it should remain indefinitely. I would strongly encourage @crhallberg to refactor this so that the value is inside a span with a class, and then we can make it look nicer with CSS. I just didn't want to mess with the styles on my own, since I figured @crhallberg would need/want to rewrite anything I did anyway. ;-)

3.) I agree that "drop up" would be a nice feature -- and probably something that wasn't implemented before because of the emphasis on placement in the header up until now.

[sturkel89]

I reviewed the test branch in comparison with the dev branch in all three themes. In this comment, I'll focus on two elements on the results view page

On dev, the two variable elements (Results per page and Sort) are in boxes that stay at a constant width regardless of the user's selection. The text is left aligned, so there's always some space between the words Author, Title, or Date ascending (etc.) and the down arrow.

On test, each box resizes depending on the user's selections. In addition, the text in the box seems to be right aligned, with minimal space between the text and the down arrow, so it seems a bit tight, especially in Bootprint3.

(NB: I am assuming that once this branch is merged into dev, the improved emphasis and highlighting behavior around the selected dropdown box or search box will be present.)

In my opinion, the design in dev provides a better user experience when the user is actively switching between different sorts. When you change your sort in dev, the contents of the box change, but your eye doesn't have to dart right and left as the length of that element gets shorter and longer.

However, if the user sticks with one selection, I think it's slightly easier on the eye to employ the variable lengths based on the length of the sort name. There needs to be adequate padding in each box, though. Sandal and Bootstrap3 are fine, but Bootprint3 is too skimpy in terms of padding

In terms of themes, Sandal does the best job of fitting the two elements into a pretty stable rectangle shape. If we could do a tiny bit of nudging for the other two themes, so that things were perfectly aligned between the words Results and Sort on the left, and the edges of the dropdown boxes on the right, that would be great.

[demiankatz]

Thanks for the feedback, @sturkel89. This sounds like a job for @crhallberg, if he has a moment to think about the CSS necessary to make this work better.

[crhallberg]

I merged the conflicts, but I need the merge work on the SavedSearchesTest checked.

[demiankatz]

Thanks, @crhallberg. Let me know if it would be helpful for me to run tests at any point.

[crhallberg]

That would be helpful, @demiankatz! I just want to make sure I did the git merge on that one test correctly. I'll take care of the new conflicts now.

[demiankatz]

@crhallberg, there are now several failing tests:

There were 4 errors:

1) VuFindTest\Mink\FavoritesTest::testListSorting
Exception: Failed to set value after 6 attempts.

/usr/local/vufind/module/VuFind/src/VuFindTest/Integration/MinkTestCase.php:589
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/FavoritesTest.php:436

2) VuFindTest\Mink\LibraryCardsTest::testSwitchingCards
Exception: Failed to set value after 6 attempts.

/usr/local/vufind/module/VuFind/src/VuFindTest/Integration/MinkTestCase.php:589
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/LibraryCardsTest.php:224

3) VuFindTest\Mink\SavedSearchesTest::testNotificationsInSearchToolbarDeduplication
Exception: Failed to get text after 6 attempts.

/usr/local/vufind/module/VuFind/src/VuFindTest/Integration/MinkTestCase.php:691
/usr/local/vufind/module/VuFind/src/VuFindTest/Integration/MinkTestCase.php:631
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/SavedSearchesTest.php:427

4) VuFindTest\Mink\SavedSearchesTest::testNotificationsInSearchHistoryDeduplication
Exception: Failed to get text after 6 attempts.

/usr/local/vufind/module/VuFind/src/VuFindTest/Integration/MinkTestCase.php:691
/usr/local/vufind/module/VuFind/src/VuFindTest/Integration/MinkTestCase.php:631
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/SavedSearchesTest.php:487

ERRORS!
Tests: 2691, Assertions: 17458, Errors: 4.

Not sure if this is related to merge errors, or if there were also some tests added to dev that need to be adjusted for compatibility with the new logic here.

[demiankatz]

@crhallberg, I noticed there were some click calls that could be simplified in the test, so I pushed up an improvement... but that did not change the test failures. Please let me know if you need me to take a deeper look at this! @EreMaijala might also have ideas if you're stuck, since I think he made the most recent test revisions.

[crhallberg]

Does someone have a setup for library cards? That part of the code got pretty complex and I'm not setup to test it.

[demiankatz]

Does someone have a setup for library cards? That part of the code got pretty complex and I'm not setup to test it.

There are instructions on library card testing here in the wiki. I've been recently trying to build up this manual testing page to help with situations like this one -- already paying off! :-)

[demiankatz]

I'm pushing all theme-related PRs that were not finished in time for 10.0 to the 11.0 milestone; it will be easier to finish these if we only have to operate on one theme at a time. I think we should establish a dev-11.0 branch fairly early in the 10.1 development process so that this work can move forward as soon as possible.