Additional options to control Alert Banner visibility #3764
Conversation
# Conflicts: # CHANGELOG.md
There was a problem hiding this comment.
So I had not thought of combining these into a single property. Appreciate the implementation here, trying to weigh pros/cons.
Pros
- Simpler, maybe, in that there's one new (replacement) top-level property on the spec.
- and still one form control in the UI.
Cons
- minor breaking change for any apps that had a preset saved with clientApps - I am not overly worried about it, there might be one or two, but if we added two new props for the new limits, the existing ones would still work
- would it be more clear to have dedicated controls for each limit? Thinking of how limits across dimensions are AND'd together, OR'd within.
- Limiting the current app version helpful in a way - although if you wanted to have a banner ready to show to an upcoming release you couldn't do it. If we had a dedicated text input you could put in current or next, etc.
- Don't want to save appVersion in presets - easier to selectively include if in dedicated field
I dunno - obviously making a case here for separate fields to some extent, but happy to discuss, do appreciate aspects of this approach
| try { | ||
| const roles = await XH.fetchJson({ | ||
| url: 'roleAdmin/list' | ||
| }); |
There was a problem hiding this comment.
This service might not always be enabled - some apps don't use Hoist's built-in role management. In that case, this would throw. Looks like we should have this call roleAdmin/config first to determine if enabled. If so, go on to load the roles. If not, might end up updating the UI, depending on how much we say about roles that's visible to users.
| {lastDismissed, onClose} = this; | ||
|
|
||
| if ( | ||
| !active || | ||
| !message || | ||
| (expires && expires < Date.now()) || | ||
| (lastDismissed && lastDismissed > publishDate) || | ||
| !this.isTargetedApp(clientApps) | ||
| isEmpty(limitTo) || |
There was a problem hiding this comment.
This looks wrong - believe instead you want all the helper methods below to return true if no limits spec'd
| /** | ||
| * Returns `true` if there are no limitations on application or if the | ||
| * current application is included in the defined limits. | ||
| * Returns `false` otherwise. |
There was a problem hiding this comment.
Nit - we can leave off the "returns false otherwise" - implied. TBH these first two private methods don't really need comments - they are pretty clear on their own. Could amend the first to isTargetedClientApp for a bit of extra clarity. A comment on the role method to indicate that user needs to have at least one of the roles might be helpful.
| label: `Role: ${role}`, | ||
| value: `role-${role}` | ||
| })) | ||
| ); |
There was a problem hiding this comment.
Would alpha-sort these if we go this route
| })) | ||
| noOptionsMessageFn: () => | ||
| 'Enter one or more app codes, app versions, or roles.', | ||
| options: limitToOptions | ||
| }), | ||
| info: fragment( | ||
| span('Specify what apps should the banner be shown to.'), |
There was a problem hiding this comment.
This help text is now mismatched
amcclain
changed the title
…version
Hoist P/R Checklist
Pull request authors: Review and check off the below. Items that do not apply can also be
checked off to indicate they have been considered. If unclear if a step is relevant, please leave
unchecked and note in comments.
developbranch as of last change.breaking-changelabel + CHANGELOG if so.If your change is still a WIP, please use the "Create draft pull request" option in the split
button below to indicate it is not ready yet for a final review.