Angular staff catalog post-3.3 omnibus

[Bill Erickson]

Work continues on the Angular staff catalog. Some working branches are starting to blend and depend on each other, so I'm opening this bug to collect the related bugs together and post a merged branch w/ code from each of these bugs:

bug #1818288 - Record detail holds tab
bug #1819053 - Record basket export feature
bug #1819498 - Call number browse
bug #1819745 - Search result title links
bug #1820304 - Volume/copy place hold links
bug #1821382 - Holdings maintenance / Conjoined Items / Initial Serials actions selector.

Branch en route.

[Bill Erickson (berick)]

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/lp1823367-ang-catalog-post-3.3-omnibus

[Bill Erickson (berick)]

The catalog work includes various general-purpose additions as well:

* Grid action menu grouping
* Grid/Editor support for ternary bools (true/false/null)
* Boolean component for common yes/no display
* Grid load-time progress indicator
* Migrating various grid actions from function handler refs to @Output() handlers.
* Grid option to display only markup-declared fields by default.
* Improved grid local sorting
* Grid option to disable paging and hide paging controls.
* Expanded Grid checkbox controls (setting / getting values)
* Grid column tooltip disable option
* Pipe version of format.transform(...) called FormatValuePipe for in-template use.
* Org select disable option
* Shared staff modules for holds, holdings, and booking

[Dan Wells (dbw2)]

Hello Bill,

As mentioned in IRC a day or two ago, I've begun reviewing these various patches. You have clearly done yeoman's work here!

One small thing, I am wondering about the naming convention we're using for EventEmitters. In cases I've seen so far, we're using an "onVerb" type name for the emitter itself, e.g. onChange, onRowClick, etc. This feels a little bit against my (rudimentary) understanding of how these are used, as you end up having something like (onClick)="onClick()", which reads (in my brain) as "on onClick, run onClick". That is, it is contrary to the normal DOM events used when a component contains a built-in DOM node, and you would use (click)="onClick()", that is, "on click, run onClick". (It is also more obvious using the "canonical" directive forms, of course, i.e. on-click="onClick()".)

In other words, if we drop the "on" from the emitter name itself, then our custom components can look and act more like the built-ins, which I think is of value. And if we're going to change our practice, better to do so now while the code is still young. Please let me know what you think!

Sincerely,
Dan

[Bill Erickson (berick)]

Thanks, Dan!

I started using (onVerb) because I was concerned (verb) might conflict with built-in events. Apparently, this can have some unexpected consequences.

https://stackoverflow.com/questions/40276400/what-are-recommanditions-for-output-event-names-to-prevent-native-event-name-c

https://medium.com/angular-athens/naming-of-output-events-in-angular-2063cad94183

We do have some that would collide if the "on" was removed, so I'm hesitant to do a global search/replace. One suggestion I saw was to prefix the @Output name with something that describes the action.

E.g. Instead of:

<eg-org-select (onChange)=.../>

You might have:

<eg-org-select (orgChange)=.../>

[Dan Wells (dbw2)]

I wondered if something like that might be the reason. Bummer.

Oddly enough, they use a lot of examples like (verb), and as a commenter pointed out, they specifically say to not use onVerb for output properties in the Angular style guide:

https://angular.io/guide/styleguide#dont-prefix-output-properties

Also from one of your links, the second half of the following statement, if accurate, worries me:

"We should not use output and input names that collide with either a native property/event name or even an input/output name used by any other component or directive."

Maybe I'm naive, but that seems to put a giant damper and the whole component reusability angle, I would think.

I'm going to need to play around with it a little more to actually get a feel for different approaches. Maybe the tried and true eg- prefix (egClick, egChange, etc.) could be another option.

P.S. I reviewed and pushed the first chunk of this branch (#1818288) to master and rel_3_3, and will continue with the next chunk. Thanks, Bill!

[Bill Erickson (berick)]

>
> Oddly enough, they use a lot of examples like (verb), and as a commenter
> pointed out, they specifically say to not use onVerb for output
> properties in the Angular style guide:
>
> https://angular.io/guide/styleguide#dont-prefix-output-properties

Yes, I'm fully on board with dropping the on's.

> Also from one of your links, the second half of the following statement,
> if accurate, worries me:
>
> "We should not use output and input names that collide with either a
> native property/event name or even an input/output name used by any
> other component or directive."
>

Yeah, I question that second-half comment. Never heard that before.
Assuming your not masking browser built-ins, I fail to see how this could
be an issue. I have many pages with numerous onChange's and I've never had
any odd behavior.

> I'm going to need to play around with it a little more to actually get a
> feel for different approaches. Maybe the tried and true eg- prefix
> (egClick, egChange, etc.) could be another option.
>

I could see that. It might also encourage consistency in how event
handlers of a given type behave.

>
> P.S. I reviewed and pushed the first chunk of this branch (#1818288) to
> master and rel_3_3, and will continue with the next chunk. Thanks,
> Bill!
>

Thanks, Dan!

[Jane Sandberg (sandbergja)]

The Holdings View treegrid looks fantastic, and I think a lot of people will be happy to have it. There are a few readability issues with the color scheme, though -- the dark blue text on dark blue background is particularly hard to make out.

[Bill Erickson (berick)]

Thanks for the input, Jane. Dan and I had a similar discussion at the conference. I think he may be experimenting some with the colors...

Tagging with pullrequest, which I thought was already there.

[Jane Sandberg (sandbergja)]

A note that there are two things in this branch that would also be really handy for the Booking Refresh:

* the disableTooltip feature for grid columns (from this commit: https://git.evergreen-ils.org/?p=working/Evergreen.git;a=commit;h=549e3be4bf3be9a448733dd617f135a6e4cf3dab)
* the option to disable paging in grids (https://git.evergreen-ils.org/?p=working/Evergreen.git;a=commit;h=47d091340979083db2758b173178366666484e90)

[Bill Erickson (berick)]

I have rebased the working combo branch to current master. I also pushed a commit to make the ident query default to ISBN searching (the first entry in the selector) as an improvement over having no value selected.

Jane or anyone who may want to improve the colors in holdings maintenance, see eg2/src/app/staff/catalog/record/holdings.component.css

[Dan Wells (dbw2)]

I have tested all but the first few commits, and I do have an alternative color scheme to propose. Since Bill no longer has this claimed, I am going to grab it to signoff on what is there and post a possible color change.

[Dan Wells (dbw2)]

Okay, pushed everything so far to master and rel_3_3. Thanks, Bill!

Also, here is a branch with four proposed follow-up commits:

working/user/dbwells/lp1823367-ang-catalog-post-3.3-omnibus-signoff-style-changes

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/dbwells/lp1823367-ang-catalog-post-3.3-omnibus-signoff-style-changes

Rather than rehash it all here, please read the commit messages for all the gory details. To sum up, basically it is an attempt to make some of the color choices more mild, and also to start documenting some style guidelines based on existing practices. It also fixes one bug preventing selection highlight from working.

I tried quite a large number of other color alternatives, so if we don't like this direction, I have at least three more on deck.

Sincerely,
Dan

[Bill Erickson (berick)]

Thanks, Dan!

I have reviewed your commits and added sign offs. I like the color changes. They're not so intense and it's easier to read the text. I have opted not to merge these just yet in case others would like to review.

Along with the sign-offs I have appended 2 more commits. One fixes a bug in the grid checkbox handler (e.g. clicking "Show Copies" in the holdings grid stopped working w/ recent grid changes). The other sets the first entry (ISBN) as the default search index under the numeric search tab.

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/lp1823367-ang-catalog-post-3.3-omnibus-continued

[Jane Sandberg (sandbergja)]

+1 to documenting style guidelines for the Angular Client. Those guidelines might be more impactful if they live on the wiki (https://wiki.evergreen-ils.org/doku.php?id=dev:angular_dev_best_practices), where they'd be more discoverable and easily understandable to folks who don't use Asciidoc. If they make more sense in the code, though, we should add links between the two documents. Thoughts?

[Dan Wells (dbw2)]

My thinking on adding the doc here is mainly as a convenient and shared place for ideas to gel a bit. I agree that the wiki would be the best eventual home, but I also don't want to add to the heap of half-baked pages on there (I've already added enough of those over the years).

Now maybe a half-baked text file buried in the code isn't much better, but at least it is trying something different :)

[Dan Wells (dbw2)]

On another note, and I meant to raise this before the horse got out of the barn, but I failed. This code adds a lot of new references to "volume" (and variations thereof) where I think, for clarity with UI decisions, we should use "call number" (and variations thereof). I am going to create a branch in that direction, but please chime in if it sounds like a bad idea. I would just rather deal with it now, before a lot of activity starts building on this, and not suffer through code/interface terminology mismatch for another entire generation of the Evergreen code.

[Bill Erickson (berick)]

I have rebased my working branch to master and pushed an additional commit to address Dan's concerns about the use of "Volume" instead of "Call Number".

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/lp1823367-ang-catalog-post-3.3-omnibus-continued

[Jane Sandberg (sandbergja)]

Dan's new color scheme is great, and so is its documentation. I pushed those four commits to master and rel_3_3.

Thanks for switching Volume -> Call Number, Bill. Could you please also switch "Copy" to "Item", for the same reason?

[Dan Wells (dbw2)]

Grabbing this to finish up the terminology changes...

[Bill Erickson (berick)]

And beware my branch (user/berick/lp1823367-ang-catalog-post-3.3-omnibus-continued) has a couple of additional fix commits besides the Call Number patch.

[Dan Wells (dbw2)]

Okay, new branch posted here:

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/dbwells/lp1823367-ang-catalog-post-3.3-omnibus-rename

working/user/dbwells/lp1823367-ang-catalog-post-3.3-omnibus-rename

This signs off on Bill's latest commits, then adds two more. The first is a straight-up relabeling change from "Copy" to "Item" in various eg2 interfaces. The second digs a little deeper at "volume" in some of the underlying code. Permit me to quote the commit message:

"It isn't really correct to use "volume" and "call number" interchangeably. This code substitutes various forms of the second for various forms of the first within the internal variable and function names. To help contain the change, it tries to only do so for values which are native to eg2, and not leaking from or into other interfaces.

Understanding that this change is somewhat disruptive, I still think it is in our overall best interest, as it will help us from falling into old habits, and it will help future coders better understand the relationship between the code and the interface."

I also feel like this is a bit of a now-or-never type change, and I much prefer now. The client isn't likely to undergo yet another major rewrite anytime soon, so this will be our best opportunity for quite some time to get our code and our user terminology on the same wavelength. If first reactions are negative, I hope folks will toss it over a few times before rejecting it.

It is also worth noting I did not do the same for Item/Copy. "Copy" is just a little more ingrained, given it's use at the DB layer, and that switch doesn't have quite the same urgency (IMHO). Not that I would necessarily mind if we ultimately did make that change, though.

[Bill Erickson (berick)]

+1 to "Call Number" label and internal migration.

I kicked the tires on the branch and only found one minor issue, for which an additional patch has been pushed along with sign-off's:

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/lp1823367-ang-catalog-post-3.3-omnibus-rename-so

[Galen Charlton (gmc)]

Merged the latest to master, including a follow-up that undoes the renaming of the eg.cat.transfer_target_vol local storage key to avoid a regression in the interaction between the AngularJS item status page and the Angular holdings view.

I'm marking this bug as fix-committed as I believe that the original bugs making up the omnibus have all been addressed. A new one should be opened for any remaining internal volume => call number or holding{s} renaming work.

Thanks, Bill, Jane, and Dan!