angular: add eg-help-popover component

Bug #1831781 reported by Galen Charlton on 2019-06-05
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Evergreen
Wishlist
Unassigned

Bug Description

The Angular staff interface should have a help popover component similar to one available in the AngularJS interface.

This is part of a project to prepare for converting some acquisitions interfaces to Angular. Work on this bug was sponsored by MassLNC, Georgia Public Library Service, Indiana State Library, CW MARS, and King County Library System.

Galen Charlton (gmc) on 2019-06-05
Changed in evergreen:
milestone: none → 3.4-beta1
Galen Charlton (gmc) wrote :

Patch(es) for this are available as part of the following branch:

working/collab/gmcharlt/angular-widget-improvements-2019-06
https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/collab/gmcharlt/angular-widget-improvements-2019-06

tags: added: angular pullrequest
Jane Sandberg (sandbej) on 2019-06-13
Changed in evergreen:
assignee: nobody → Jane Sandberg (sandbej)
Jane Sandberg (sandbej) wrote :

Thanks for this new component, Cesar and Galen. It works well, and is a great addition to the Angular client. I also appreciate all the sandbox examples.

A few thoughts:

1) Other components in the Angular client have a comment over most @Input lines with some information about what it does. For example, see https://github.com/evergreen-library-system/Evergreen/blob/master/Open-ILS/src/eg2/src/app/share/org-select/org-select.component.ts. I find these really helpful. Could you please add these?

I think it might also be worth adding this practice to this doc: https://wiki.evergreen-ils.org/doku.php?id=dev:angular_dev_best_practices

2) The unit test looks like a boilerplate Angular component unit test. I don't think it adds much value, and I would lean toward not including it. I'm open to other thoughts, though.

3) When I hover over the popover button with my mouse, the cursor changes into an I-beam. Since it is a button, rather than a piece of text, a pointer seems like a more appropriate cursor choice. A quick, semantic way to do this (while also canceling out the need for the tabindex attribute) would be to wrap the span in a <button class="btn"></button>.

4) The accessible name of the popover is set to "live_help" -- not very helpful (and potentially even misleading). Adding an aria-label with what the popover actually does would fix this. More info about checking the accessibility tree/accessible names in Chrome here: https://developers.google.com/web/tools/chrome-devtools/accessibility/reference#pane

Changed in evergreen:
assignee: Jane Sandberg (sandbej) → nobody
Galen Charlton (gmc) wrote :

I've pushed to the collab branch a follow-up that:

    - Wrap the image in a button; this removes the need for setting
      tabindex and makes the cursor display as a pointer when it
      is over the popover.
    - Add aria-label attributes
    - add some usage comments

I've chosen to leave the test as is. While I agree that it currently doesn't do much that wouldn't already be caught by the compiler, its presence slightly reduces the effort for anybody to add future (presumably regression) tests.

Jane Sandberg (sandbej) wrote :

Thanks for this excellent component, Cesar, Galen, and Bill. Pushed to master -- along with the follow-up and Bill's signoff -- for inclusion in 3.4.

Changed in evergreen:
status: New → Fix Committed
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers