ViewManager Enhancements #3836
ViewManager Enhancements #3836
Conversation
+ support for Global label + remove autoSave + Rename 'shared' to 'global'
…3838) + Report connectivity issues more quickly. + Allow values to be optionally configured via `xhAdminAppConfig`.
|
|
||
| ### ⚙️ Technical | ||
| * Misc. Improvements to ViewManager | ||
| ### 💥 Breaking Changes (upgrade difficulty: 🟢 LOW - changes to new ViewManager APIs) |
There was a problem hiding this comment.
I think we could take a liberty here and not book this as a breaking change, especially if we have no other reason to jump to a new major version.
| * Improvements to ViewManager API and functionality. | ||
| * Replace `autoSave` with option to persist pending view. | ||
| * More customization options. | ||
| * Handle deleted and update collisions more gracefully. |
| /** Either null for private views or special token "*" for globally shared views. */ | ||
| acl: '*' | null; | ||
| dateCreated: number; | ||
| export interface ViewInfo { |
There was a problem hiding this comment.
I'm surprised that I can't think of another term to use here (vs Info) - casting about for something that we've already used in an analogous context. I am sure you guys discussed at length...
I don't think our go-to Spec applies, since this is not a "recipe to make one". Did you consider Meta or Metadata?
Not at all critical, happy to proceed with this, but one of those cases where I feel like I can't quite put my finger on the word that captures the concept.
There was a problem hiding this comment.
Agreed - we spent a good amount of time trying to come up with the right name, and not sure we quite landed it. Originally, I had View, State and ViewWithState. That was a little wonky. I had the same though re: spec. I thought of Meta or ViewMeta and still think that fits reasonably well.
| enableSharing?: Thunkable<boolean>; | ||
| /** Used to persist the user's last selected + favorite views and autoSave preference. */ | ||
| persistWith?: PersistOptions; | ||
| manageGlobal?: Thunkable<boolean>; |
There was a problem hiding this comment.
This reads as a bit too terse for me - just barely, but would be nice for it to be as clear as possible. Could we do manageGlobalViews? I know that "views" could be considered implied - it's a view manager - but still...
Would really like enableGlobalViewManagement but even I admit that's pretty long. An earlier iteration of project used canManageGlobal - not sure if the can prefix is something we use anywhere else.
Alternatively, any sense we want an object here like enableSharing: {global: boolean}? If we were more sure of our next steps with per-user sharing maybe we would know.
Again, don't need to change, this just stuck out at me.
| persistWith?: PersistOptions; | ||
| manageGlobal?: Thunkable<boolean>; | ||
|
|
||
| /** Used to persist the user's last selected view + favorite views */ |
| manageGlobal?: Thunkable<boolean>; | ||
|
|
||
| /** Used to persist the user's last selected view + favorite views */ | ||
| persistWith?: ViewManagerPersistOptions; | ||
| /** | ||
| * Required discriminator for the particular class of views to be loaded and managed by this | ||
| * model. Maps onto the `type` field of the persisted `JsonBlob`. Set to something descriptive | ||
| * and specific enough to be identifiable and allow for different viewManagers to be added | ||
| * to your app in the future - e.g. `portfolioGridView` or `tradeBlotterDashboard`. | ||
| */ | ||
| viewType: string; |
There was a problem hiding this comment.
Should we drop leading view here as well?
| /** Pending (transient) view, can include not-yet-saved changes. */ | ||
| @observable.ref pendingView: View<T> = this.view; | ||
| /** Loaded saved view library - both private and global */ | ||
| @observable.ref views: ViewInfo[] = []; |
There was a problem hiding this comment.
No doubt also something you discussed, but wish we didn't have the singular property be one type and the plural of the same name be the other... viewInfos pretty wonky though, eh?
| /** True if any async tasks are pending. */ | ||
| get isLoading(): boolean { | ||
| const {loadModel, saveTask, selectTask} = this; | ||
| return loadModel.isPending || saveTask.isPending || selectTask.isPending; |
Should have most of the remaining items, outside of the change to persist the pending view, which I will add in the am
See toolbox PR as well
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.