Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Menubar - Add "find menu item" search feature #16597

Merged
merged 1 commit into from
Feb 26, 2020

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Feb 19, 2020

Overview

Allows user to locate menu items by typing a few letters.
Find tool is located under the "Home" (Civi logo) menu.

Before

No finder for menu items.

After

The first item in the "Home" menu is a finder to locate menu items. Accesskey "M"

Screenshot from 2020-02-21 18-45-46

Typing a few letters reveals a sub-menu of clickable results:

Screenshot from 2020-02-21 18-46-35

Comments

This obsoletes the QuickSearch Extension.

@civibot
Copy link

civibot bot commented Feb 19, 2020

(Standard links)

@civibot civibot bot added the master label Feb 19, 2020
@twomice
Copy link
Contributor

twomice commented Feb 19, 2020

So. Freakin. Cool.

@eileenmcnaughton
Copy link
Contributor

@twomice any chance you can give it a spin? Code passes the 'nothing looks worrying test'

@twomice
Copy link
Contributor

twomice commented Feb 19, 2020

Sure, will give it a go.

@twomice
Copy link
Contributor

twomice commented Feb 20, 2020

@colemanw If you can ping me when tests are passing, I'll give it a review.

@colemanw
Copy link
Member Author

@twomice the 4 test failures are unrelated. Those look like flaky time-based tests which fail at certain times of the day/week/month.

@twomice
Copy link
Contributor

twomice commented Feb 20, 2020

@colemanw When I type, an "x" control appears in the text field, but that x does nothing when I click on it. (Otherwise looks good so far.)
x

@colemanw
Copy link
Member Author

@twomice I just noticed that too. Looks to be a side-effect of the menu's markup and the <input> being nested within an <a> tag which is probably not valid html. It also caused this other problem: https://lab.civicrm.org/dev/core/issues/1525

The "right" fix is somewhat elusive but for now I've just changed the input type from "search" to "text" so the non-functional x won't appear.

@eileenmcnaughton
Copy link
Contributor

test this please

@MegaphoneJon
Copy link
Contributor

I know that full keyboard shortcut support is a heavy lift for Civi at the moment - but based on my use of the Coffee module for Drupal, I'm guessing that if this had a keyboard shortcut it would quickly become a favorite tool of Civi power users everywhere.

Allows user to locate menu items by typing a few letters.
Find tool is located under the "Home" (Civi logo) menu.
@colemanw
Copy link
Member Author

@MegaphoneJon great idea! I looked into it and actually there is already a shortcut key to the "Home" (Civi logo) menu (ALT + M) which wasn't really doing anything except opening that item. I just moved the search box to that menu item (arguably a better place for it) and now the ALT + M hotkey will take you straight to the finder. (note the hotkey could be ALT+SHIFT or something else depending on the browser/OS).

@twomice
Copy link
Contributor

twomice commented Feb 22, 2020

One problem with using ALT+M for this is that now I can't access "hide civicrm menu" via the keyboard, nor or other sub-items of the "Civi logo" menu. Not a deal breaker as far as I'm concerned, but it does have the effect of removing all o the previous utility of this shortcut in favor of different functionality.

I don't know how we could measure the potential impact of this change (we probably don't have usage stats on this shortcut key), but thought it worth mentioning. @colemanw , any thoughts?

@colemanw
Copy link
Member Author

@twomice I just tried it and was able to access the "hide menu" item with ALT + M TAB TAB.

@twomice
Copy link
Contributor

twomice commented Feb 22, 2020

I'm cool with that if you are. Reviewing now.

@@ -1,4 +1,9 @@
{
"com.civibridge.quickmenu": {
"obsolete": "5.24",
"disable": true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@colemanw Wasn't aware of "obsolete" capability. Kinda cool. Is this documented somewhere?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No it's fairly new. So far its being used for extensions whose features have been migrated into core (Api4, KAM, ExportUI, etc.). Adding an extension to this list will:

  1. Automatically disable/uninstall it during the upgrade.
  2. Not allow it to be reenabled; label it "obsolete" in the UI.
  3. Resolve any dependencies (so an extension that declares a dependency on Api4 will still work)

Let me see if I can find a spot for that in the dev docs pages...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@twomice twomice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(CiviCRM Review Template WORD-1.2)

  • General standards
    • (r-explain) Pass: Explanation is clear and thorough.
    • (r-user) Pass: Minor usability impact (lost use of arrow keys to access sub-menu items with ALT+M shortcut key) is mitigated with workaround (use of Tab/Shift+Tab instead)
    • (r-doc) Pass: ALT+M Shortcut key is documented in "Access Keys" help popup.
    • (r-run) Pass: I used this feature on the auto-generated test website (http://core-16597-6w4xq.test-1.civicrm.org:8001/ at time of writing) and observed it works as expected.
  • Developer standards
    • (r-tech) Pass: Additional UI-only functionality with dfeined and limited scope, not impacting existing features.
    • (r-code) Pass
    • (r-maint) Pass
    • (r-test) Pass

@twomice
Copy link
Contributor

twomice commented Feb 22, 2020

@eileenmcnaughton See my review above. I believe this is ready to merge.

@eileenmcnaughton eileenmcnaughton added the merge ready PR will be merged after a few days if there are no objections label Feb 24, 2020
@eileenmcnaughton
Copy link
Contributor

I've given this merge-ready - I just want to give @aydun & @michaelmcandrew - who was the other person who was involved in usability over your way? a chance to see the notes about the keyboard shortcut changes

@michaelmcandrew
Copy link
Contributor

hey @eileenmcnaughton - it was claire williams from www.visibility.org.uk - I've sent her a quick email.

@aydun
Copy link
Contributor

aydun commented Feb 24, 2020

I like the new feature - slight concern re the arrow keys as documented above but doesn't seem a blocker to me.

Occasionally it brings up duplicates: eg entering 'cam' brings up 4 links for Personal Campaign Pages, 2 each for events and contribute - but I'm guessing that is related to the underlying menu items under Contributions and Events being duplicated under Administer.

Also looping in @JoeMurray re accessibility.

@colemanw
Copy link
Member Author

I get the concern about the arrow keys but I'm not sure anything can be done about it. The issue is that the search input is a text field, and pretty much every UI convention says that when a text field is focused, the arrow keys are for moving the cursor, not for moving focus to a different element. FYI the quicksearch field has the exact same issue.

And yes, the menubar is full of duplicates. Probably to make things easier to find... because of the lack of a search box!

@seamuslee001
Copy link
Contributor

This looks good to me as well and will be a nice improvement

@yashodha
Copy link
Contributor

@colemanw @seamuslee001 tested this and works well 👍
report

@yashodha
Copy link
Contributor

@colemanw Shall we merge or we waiting on more feedback/changes?

@colemanw
Copy link
Member Author

I think this is ready to merge.

@eileenmcnaughton
Copy link
Contributor

OK - I'll merge - if we get further feedback from @michaelmcandrew's email / pings we can address

@eileenmcnaughton eileenmcnaughton merged commit 8a4a1be into civicrm:master Feb 26, 2020
@eileenmcnaughton eileenmcnaughton deleted the drilldown branch February 26, 2020 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants