Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New ViewManager for persisting named bundles of grid and other component state #3774

Closed
wants to merge 26 commits into from

Conversation

Ryanseanlee
Copy link
Contributor

@Ryanseanlee Ryanseanlee commented Sep 4, 2024

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.

  • Caught up with develop branch as of last change.
  • Added CHANGELOG entry, or determined not required.
  • Reviewed for breaking changes, added breaking-change label + CHANGELOG if so.
  • Updated doc comments / prop-types, or determined not required.
  • Reviewed and tested on Mobile, or determined not required.
  • Created Toolbox branch / PR, or determined not required.

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.

Pull request reviewers: when merging this P/R, please consider using a squash commit to
collapse multiple intermediate commits into a single commit representing the overall feature
change. This helps keep the commit log clean and easy to scan across releases. PRs containing a
single commit should be rebased when possible.

Remaining TODOs:

  • Restore view function when Default view is selected

value,
meta,
description
}: {
type: string;
name: string;
acl?: string | PlainObject;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why PlainObject?

saveDialog({omit: !saveDialogModel})
);
render({model, ...props}) {
return fragment(defaultMenu({...props}), manageDialog(), saveDialog());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To discuss - either exporting the dialogs, exposing more props for customizing, or making them pluggable (i.e. PersistenceManagerProps could accept a manageDialogCmp and saveDialogCmp that default to our Hoist implementations)

@@ -114,7 +129,8 @@ const objMenu = hoistCmp.factory<PersistenceManagerProps>({
}),
menuItem({
icon: Icon.refresh(),
text: 'Reset Defaults',
text: 'Reset Default View',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discuss today - Reset vs Restore. Or maybe there’s a better phrasing entirely?

/** Fn to produce a new, empty object - can be async. */
newObjectFnAsync?: () => T;
/** Optional flag to force selection of a view. Defaults false*/
allowEmpty?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding this - interested in discussing as a group this afternoon

@@ -229,6 +218,7 @@ export class PersistenceManagerModel<T extends PlainObject = PlainObject> extend

this.setPendingValue(value);
await this.onChangeAsync(value);
this._loadedInitially = true;
Copy link
Contributor

@ghsolomon ghsolomon Sep 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be nice if we can leverage the LoadSupport API to check if we’ve already completed a successful load.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That way we wouldn’t need this update this observable and could just have a getter.

isFavorite: boolean;
}
| {
type: 'divider';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would remove divider. To me, that’s a view-level concern and not a concern of the model / data.

Copy link
Contributor Author

@Ryanseanlee Ryanseanlee Sep 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without 'divider' how would we know where to put in the menuDivider? we could add a group prop to PersistenceViewTree and have some logic on the component?

// Persistence Provider
// Pass this to models that implement `persistWith` to include their state in the view.
//------------------------
readonly provider: PersistOptions = {
Copy link
Contributor

@ghsolomon ghsolomon Sep 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wonder if we should make a new PersistenceManagerProvider class that extends PersistenceProvider so the API is consistent with DashViewProvider.

@amcclain amcclain changed the title Persistence Manager New ViewManager for persisting named bundles of grid and other component state Oct 4, 2024
@amcclain
Copy link
Member

Latest process tracked @ #3821

@amcclain amcclain closed this Oct 31, 2024
@amcclain amcclain deleted the persistenceManager branch November 15, 2024 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants