Component: confirmation menu #2723

crhallberg · 2023-02-23T20:12:15Z

Are you sure you want to do this?

demiankatz · 2023-02-23T21:05:52Z

Thanks for getting this started, @crhallberg -- please let me know when you're ready for a review! Looks like there are some automated check things that need to get sorted out, so for now I'll let you work on those before I add to the to-do list. ;-)

crhallberg · 2023-03-08T21:18:05Z

I'm hesitant about reimplementing the ARIA components from scratch but up until today I hadn't found a good non-React library. Today I found goodguyry's AriaComponents repository.

I think these are a good step up from the Javascript W3C provides and they seem thoroughly tested and maintained. The only warning that I have is that the demo page is using the latest release, which is a year old. I manually tested the dev branch (updated very recently) and it so far fixes all of my issues with the demo (such as down-arrow events on the Tabs interface).

I think this would be a good thing to adopt immediately, seeing as it doesn't require a build step, only modern JS importing.

cc. @demiankatz @EreMaijala (please cc others)

crhallberg · 2023-03-08T21:57:00Z

I've dug even deeper now and the results aren't as promising as expected. It seems that they deprecated MenuButton and MenuBar and say vaguely that it's best to "build your own using the Disclosure or Dialog component", but there aren't any examples of this and no instruction for MenuBar.

demiankatz · 2023-03-09T11:30:16Z

@crhallberg, thanks for the research here! As we discussed last week, I'd be inclined to split this work into two parts:

Part 1: refactor the existing yes/no drop-down functionality to a component template, so we can standardize and centralize the current behavior.

Part 2: rewrite the refactored component template to use more modern behavior.

Since the "part 2" seems like it still requires research, I'd favor getting part 1 done for release 9.0 and then finishing part 2 later as time permits. (And of course, maybe there's a "part 1.5" where we improve the accessibility of the existing behavior without a full rewrite of everything).

demiankatz · 2023-03-10T22:01:18Z

EDIT: never mind -- the test failure I was seeing seems unrelated to this branch; investigating now, but I'll test this more thoroughly next week once whatever else is wrong gets resolved.

EDIT 2: I restarted my test environment and everything is passing now. Just a passing glitch, apparently.

demiankatz

Thanks again, @crhallberg. I've given this a closer look and found a few things to discuss. I haven't tried any hands-on testing yet, but I'll do that once we get through the more theoretical parts of the conversation. :-)

themes/bootstrap3/less/components/ui/confirm-menu.less

themes/bootstrap3/templates/search/home.phtml

themes/bootstrap3/templates/_ui/components/confirm-menu.phtml

demiankatz

Thanks for the progress here, @crhallberg -- I think that all but one of my prior review comments are now addressed... so next steps are:

1.) Resolve conflicts (looks like just a "merge and recompile CSS" situation)
2.) Begin refactoring existing confirm menus to use this component
3.) Make sure tests pass

Let me know how I can help!

themes/bootstrap3/templates/_ui/components/confirm-menu.phtml

demiankatz · 2023-03-22T11:03:40Z

@crhallberg, I took a moment this morning to merge the latest dev into this branch to make sure styles are all good after recent changes (fortunately, they are).

I also tried to start using the component to replace existing confirm menus, but I think several key features are currently missing. The first instance of a confirm menu that I could find is in the cart (cart/cart.phtml, which is also the subject of #2669 which started this whole process) for the delete buttons. I started by adding:

        <?=
          $this->component(
            'confirm-menu',
            [
              'label' => $this->transEsc('Delete'),
              'toggleClassAttr' => 'toolbar-btn dropdown-toggle',
            ]
          )
        ?>

Above the existing delete code. The button doesn't open a menu (maybe it doesn't work inside a lightbox, or I missed some important attribute?) and it also lacks capabilities for including an icon inside the button, attaching an ID to the button, or attaching IDs to the contents of the drop-down menu. I assume that some or all of these things are required in order to refactor functionality like the cart to use this component. (But I apologize if I'm missing/misunderstanding something).

demiankatz · 2023-06-01T16:26:31Z

@crhallberg, I've merged the latest dev into this PR to bring things up to date. There's still a lot of work to be done here; we'll discuss on next week's Community Call.

…rity ids (vufind-org#2723)

demiankatz · 2023-11-08T21:05:45Z

I've done another round of merging and conflict resolution to bring this up to date, but several things seem to be broken at the moment. Here's the current test output:

There was 1 error:

1) VuFindTest\Integration\View\Helper\Root\ComponentTest::testConfirmMenu
Undefined array key "class"

/usr/local/vufind/themes/bootstrap3/templates/_ui/components/confirm-menu.phtml:7
/usr/local/vufind/vendor/laminas/laminas-view/src/Renderer/PhpRenderer.php:526
/usr/local/vufind/module/VuFind/src/VuFind/View/Helper/Root/Component.php:65
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/View/Helper/Root/ComponentTest.php:113
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/View/Helper/Root/ComponentTest.php:135
/usr/local/vufind/module/VuFind/src/VuFindTest/Feature/AutoRetryTrait.php:104

--

There were 6 failures:

1) VuFindTest\Mink\HoldsTest::testCancelHold
Element not found: .alert.alert-success index 0
Failed asserting that null is of type "object".

/usr/local/vufind/module/VuFind/src/VuFindTest/Integration/MinkTestCase.php:407
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/HoldsTest.php:386
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/HoldsTest.php:422
/usr/local/vufind/module/VuFind/src/VuFindTest/Feature/AutoRetryTrait.php:104

2) VuFindTest\Mink\HoldsTest::testFrozenHoldEditingWithCancellation
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'1 request(s) were successfully canceled'
+'1 hold(s) updated'

/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/HoldsTest.php:644
/usr/local/vufind/module/VuFind/src/VuFindTest/Feature/AutoRetryTrait.php:104

3) VuFindTest\Mink\IlsActionsTest::testCancelAllIllRequest
Element not found: .alert.alert-success index 0
Failed asserting that null is of type "object".

/usr/local/vufind/module/VuFind/src/VuFindTest/Integration/MinkTestCase.php:407
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/IlsActionsTest.php:251
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/IlsActionsTest.php:452
/usr/local/vufind/module/VuFind/src/VuFindTest/Feature/AutoRetryTrait.php:104

4) VuFindTest\Mink\IlsActionsTest::testCancelSelectedIllRequest
Element not found: .alert.alert-success index 0
Failed asserting that null is of type "object".

/usr/local/vufind/module/VuFind/src/VuFindTest/Integration/MinkTestCase.php:407
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/IlsActionsTest.php:214
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/IlsActionsTest.php:465
/usr/local/vufind/module/VuFind/src/VuFindTest/Feature/AutoRetryTrait.php:104

5) VuFindTest\Mink\IlsActionsTest::testCancelAllStorageRetrievalRequest
Element not found: .alert.alert-success index 0
Failed asserting that null is of type "object".

/usr/local/vufind/module/VuFind/src/VuFindTest/Integration/MinkTestCase.php:407
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/IlsActionsTest.php:251
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/IlsActionsTest.php:526
/usr/local/vufind/module/VuFind/src/VuFindTest/Feature/AutoRetryTrait.php:104

6) VuFindTest\Mink\IlsActionsTest::testCancelSelectedStorageRetrievalRequest
Element not found: .alert.alert-success index 0
Failed asserting that null is of type "object".

/usr/local/vufind/module/VuFind/src/VuFindTest/Integration/MinkTestCase.php:407
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/IlsActionsTest.php:214
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/IlsActionsTest.php:539
/usr/local/vufind/module/VuFind/src/VuFindTest/Feature/AutoRetryTrait.php:104

ERRORS!
Tests: 2568, Assertions: 14582, Errors: 1, Failures: 6.

demiankatz

I'm currently running through all open PRs to prevent outdated PHP version 7 comments from bleeding back into the project. See comment below -- just flagging this so we fix it before merging.

module/VuFind/tests/integration-tests/src/VuFindTest/View/Helper/Root/ComponentTest.php

demiankatz · 2024-03-19T11:56:24Z

Thanks for the latest progress, @crhallberg. I resolved conflicts again and ran the test suite. Here's the current state of test failures:

There were 5 errors:

1) VuFindTest\Mink\HoldsTest::testCancelHold
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:610
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/HoldsTest.php:473
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/HoldsTest.php:509

2) VuFindTest\Mink\IlsActionsTest::testCancelAllIllRequest
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:610
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/IlsActionsTest.php:254
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/IlsActionsTest.php:451

3) VuFindTest\Mink\IlsActionsTest::testCancelSelectedIllRequest
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:610
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/IlsActionsTest.php:217
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/IlsActionsTest.php:464

4) VuFindTest\Mink\IlsActionsTest::testCancelAllStorageRetrievalRequest
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:610
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/IlsActionsTest.php:254
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/IlsActionsTest.php:525

5) VuFindTest\Mink\IlsActionsTest::testCancelSelectedStorageRetrievalRequest
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:610
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/IlsActionsTest.php:217
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/IlsActionsTest.php:538

--

There were 2 failures:

1) VuFindTest\Mink\HoldsTest::testFrozenHoldEditingWithCancellation
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'1 request(s) were successfully canceled'
+'1 hold(s) updated'

/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/HoldsTest.php:731

2) VuFindTest\Integration\View\Helper\Root\ComponentTest::testConfirmMenu
Failed asserting that false is true.

/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/View/Helper/Root/ComponentTest.php:141
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/View/Helper/Root/ComponentTest.php:170

ERRORS!
Tests: 2733, Assertions: 17580, Errors: 5, Failures: 2.

demiankatz · 2024-04-02T17:39:58Z

Now that #3545 has been merged, there are some new conflicts to resolve here. Not sure if @crhallberg or @EreMaijala would be best positioned to do that, but either way, I'm happy to help if my help is needed... just let me know! I'll likely prioritize other tasks until somebody explicitly asks me to look at this one. :-)

crhallberg · 2024-05-07T17:35:54Z

It appears to me that the confirm-menu component in this PR has been superseded by the confirm-button component from #3545. I wanted to make sure that was the case as I compare the differences and perhaps apply the tests to confirm-button.

demiankatz · 2024-05-07T17:41:27Z

@crhallberg, I believe that is correct. I didn't notice that the component had a different name in #3545 than it did here; sorry for failing to catch that! I believe the intent was to refactor the code in #3545 while leaving the underlying mechanics intact, and then use this PR to revise the component to use more modern methods.

demiankatz · 2024-06-14T11:03:02Z

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.

feat(confirm): initial code for confirm-menus.

adaefe5

crhallberg marked this pull request as draft February 23, 2023 20:12

refactor: technical debt.

4002005

crhallberg added 4 commits March 9, 2023 10:17

style: jshint and eslint.

f9af6d6

feat(aria): keyboard control from confirmation menu.

052e897

Merge remote-tracking branch 'origin/dev' into component-confirmation

876db01

style: template indenting.

2ee12ef

crhallberg marked this pull request as ready for review March 10, 2023 17:56

Merge remote-tracking branch 'origin/dev' into component-confirmation

d67e2e2

demiankatz requested changes Mar 13, 2023

View reviewed changes

demiankatz mentioned this pull request Mar 14, 2023

Add support for purging loans from history. #2779

Merged

3 tasks

crhallberg added 3 commits March 14, 2023 15:34

fix: oops all test code!

70af9b5

fix: more rails for values.

3685b4e

fix: lessToSass includes all files in components/.

88ab90d

demiankatz requested changes Mar 17, 2023

View reviewed changes

themes/bootstrap3/templates/_ui/components/confirm-menu.phtml Outdated Show resolved Hide resolved

demiankatz added this to the 9.0 milestone Mar 17, 2023

crhallberg and others added 3 commits March 17, 2023 12:34

Merge remote-tracking branch 'origin/dev' into component-confirmation

9236347

style: variable names include esc or attr.

d52cb1d

Merge branch 'dev' into component-confirmation

8ab6602

crhallberg and others added 5 commits March 22, 2023 13:18

fix: menu margin.

3461569

fix: keypress to close the confirm-menu

0185f8b

test: interacting with a component.

baf0f3f

Merge remote-tracking branch 'origin/dev' into component-confirmation

9529dcf

lessToSass.

cb6fbc4

Merge branch 'dev' into component-confirmation

ed3675b

demiankatz modified the milestones: 9.1, 10.0 Aug 1, 2023

Merge remote-tracking branch 'origin/dev' into component-confirmation

e028bd8

EreMaijala pushed a commit to EreMaijala/vufind that referenced this pull request Oct 13, 2023

[FINNA-1136] Return id as is if no regex rules has been set for autho…

a27004c

…rity ids (vufind-org#2723)

sturkel89 added the new feature label Oct 26, 2023

demiankatz added architecture pull requests that involve significant refactoring / architectural changes and removed new feature labels Nov 7, 2023

demiankatz added 2 commits November 8, 2023 15:53

Merge branch 'dev' into component-confirmation

2151122

php-cs-fixer

fa3c83b

demiankatz requested changes Feb 27, 2024

View reviewed changes

module/VuFind/tests/integration-tests/src/VuFindTest/View/Helper/Root/ComponentTest.php Outdated Show resolved Hide resolved

crhallberg and others added 5 commits March 1, 2024 14:44

Merge remote-tracking branch 'origin/dev' into component-confirmation

f2c5bae

fix: missed merge.

37c9cd8

docs: PHP version.

1ef386f

feat: better default value handling.

100c2f9

Merge branch 'dev' into component-confirmation

758a29e

EreMaijala mentioned this pull request Mar 25, 2024

Add confirm button component #3545

Merged

crhallberg added 2 commits April 22, 2024 12:58

Merge remote-tracking branch 'origin/dev' into component-confirmation

cf9e585

Merge remote-tracking branch 'origin/dev' into component-confirmation

4d03ff5

demiankatz modified the milestones: 10.0, 11.0 Jun 14, 2024

demiankatz changed the base branch from dev to dev-11.0 August 27, 2024 10:45

demiankatz changed the base branch from dev-11.0 to dev November 1, 2024 17:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Component: confirmation menu #2723

Component: confirmation menu #2723

crhallberg commented Feb 23, 2023 •

edited

Loading

demiankatz commented Feb 23, 2023

crhallberg commented Mar 8, 2023 •

edited

Loading

crhallberg commented Mar 8, 2023

demiankatz commented Mar 9, 2023

demiankatz commented Mar 10, 2023 •

edited

Loading

demiankatz left a comment

demiankatz left a comment

demiankatz commented Mar 22, 2023 •

edited

Loading

demiankatz commented Jun 1, 2023

demiankatz commented Nov 8, 2023

demiankatz left a comment

demiankatz commented Mar 19, 2024

demiankatz commented Apr 2, 2024

crhallberg commented May 7, 2024

demiankatz commented May 7, 2024

demiankatz commented Jun 14, 2024

Component: confirmation menu #2723

Are you sure you want to change the base?

Component: confirmation menu #2723

Conversation

crhallberg commented Feb 23, 2023 • edited Loading

demiankatz commented Feb 23, 2023

crhallberg commented Mar 8, 2023 • edited Loading

crhallberg commented Mar 8, 2023

demiankatz commented Mar 9, 2023

demiankatz commented Mar 10, 2023 • edited Loading

demiankatz left a comment

Choose a reason for hiding this comment

demiankatz left a comment

Choose a reason for hiding this comment

demiankatz commented Mar 22, 2023 • edited Loading

demiankatz commented Jun 1, 2023

demiankatz commented Nov 8, 2023

demiankatz left a comment

Choose a reason for hiding this comment

demiankatz commented Mar 19, 2024

demiankatz commented Apr 2, 2024

crhallberg commented May 7, 2024

demiankatz commented May 7, 2024

demiankatz commented Jun 14, 2024

crhallberg commented Feb 23, 2023 •

edited

Loading

crhallberg commented Mar 8, 2023 •

edited

Loading

demiankatz commented Mar 10, 2023 •

edited

Loading

demiankatz commented Mar 22, 2023 •

edited

Loading