wishlist: eg-grid should hide/disable "save settings" when persist key is intentionally missing

[Andrea Neiman]

Master-ish (3.3+)

If, for whatever reason, a grid is intentionally designed to NOT save settings then the "save settings" option should be removed or disabled.

This came up in testing for some grid elements during the Angular Acq project.

[Jane Sandberg (sandbergja)]

Thanks for reporting this, Andrea.

My branch for the booking refresh includes a disableSaveSettings option: https://git.evergreen-ils.org/?p=working/Evergreen.git;a=commitdiff;h=aa9ff56f31c2cca04e7ee40293925cf84eaece3c#patch8

It disables (grays out) the Save option from the column picker. But I was not sure if it would be better to disable or hide the Save option. Thoughts?

[Galen Charlton (gmc)]

Thanks, Jane! I don't have a strong preference either way, but I have a mild preference for hiding it. My reasoning is that disabling a control is useful in cases where there's a way for a user or Evergreen admin to enable it through normal configuration action (like adding a permission or setting a library setting or the like). A control should be hidden (or *ngIf'ed away) if the only way to make it active is a code change.

[Jane Sandberg (sandbergja)]

That makes sense to me, Galen. I will update my branch accordingly; hopefully sometime this week.

[Galen Charlton (gmc)]

Thanks! Also, looking at the patch again, I have a question: is disableSaveSettings strictly necessary? I'm thinking that simply checking whether persistKey is set might be sufficient (and even if there is a reason to have a separate disableSaveSettings attribute, persistKey should be checked as well).

[Jane Sandberg (sandbergja)]

Relying simply on persistKey seems reasonable to me.

[Jason Boyer (jboyer)]

I also really like hiding things that user's can't cause to be enabled by changing something on the page while they're looking at it. +1 to that.

Following Galen's question about hiding the save options when persistKey is missing, I'd like to suggest 1 more wrinkle in that discussion. Hiding save options if pK is missing is a UI win because no one is frustrated about silent failures, but to address the same point that you were with disableSaveSettings (the "intentionally" part) you could also consider 1 specific persistKey value to also mean "don't save." Something like persistKey='disabled' or similar. That way both intentional states ("please save this" and "we don't want to save this") are contained in the same setting key and it makes conflicting states like (disableSaveSettings='true', persistKey='eg.circ.please.save') impossible while also signaling that no, we did not forget to set persistKey.

That said, my feelings on this aren't super strong; I just wanted to throw that thought out there if you were going to be looking at any other potential changes. :)

[Galen Charlton (gmc)]

+1 to persistKey='disabled'

[Jane Sandberg (sandbergja)]

I created a branch called user/sandbergja/lp1828840_ng_grid_no_save_button_without_pkey that implements Jason's approach.

Here's the link: https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/sandbergja/lp1828840_ng_grid_no_save_button_without_pkey

And here are the testing notes from the commit:

1) Apply this commit.
2) Create an eg-grid in the Angular staff client without a persistKey
attribute (or use the one in the sandbox).
3) Compile the client and open the grid in your browser. Open the
column picker menu. Note that the Save button does not display.
4) Add an arbitrary value to the persistKey attribute.
5) Repeat step 3. Note that the Save button does display.
6) Change the value of the persistKey attribute to "disabled".
7) Repeat step 3. Note that the Save button does not display.

[Galen Charlton (gmc)]

Works for me. Signoff branch is user/gmcharlt/lp1828840_signoff.

[Bill Erickson (berick)]

Looks good. Thank All. Merged to 3.3+