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

Update GroupingChooser with local model for transient UI state #3420

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

haynesjm42
Copy link
Member

Fixes #3398

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.

@haynesjm42 haynesjm42 requested a review from lbwexler July 18, 2023 15:34
@@ -39,6 +40,126 @@ export interface GroupingChooserProps extends ButtonProps<GroupingChooserModel>
styleButtonAsInput?: boolean;
}

class GroupingChooserLocalModel extends HoistModel {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is duplicated in the mobile component - didn't want to export this model but not sure how we can share it between the 2 packages ... should we just export and put a comment as "Not for application use"?

Copy link
Member

Choose a reason for hiding this comment

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

you can put it in an /impl directory, and mark it as @internal. We do this all the time!

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the tip, I knew we must have a pattern for this (and do this all the time in apps). Was hoping for something that would hide the class to app code, doesn't seem like there is any enforcement of @internal . Looks like there are lots of proposals out there for this kind of thing, hopefully something makes it in eventually.

Looks like there is a way to strip these from d.ts output - https://www.typescriptlang.org/tsconfig#stripInternal - though since we are full typescript I don't think this does anything.

}

const dimensionList = hoistCmp.factory<DimensionListProps>({
render({model, emptyText, impl}) {
Copy link
Member

Choose a reason for hiding this comment

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

same as above

Copy link
Member Author

Choose a reason for hiding this comment

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

Suggesting to move this into a shared impl directory? Can't really do that unfortunately as it's child components use platform-specific components

Copy link
Member

Choose a reason for hiding this comment

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

discussed with John -- there was a misunderstanding -- just mean that we should be passing the state down to the local components as dimensionList({imodel: impl}). That way they have typed access to everything.

@amcclain amcclain changed the title Grouping chooser local model Update GroupingChooser with local model for transient UI state Sep 1, 2023
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.

GroupingChooser should keep transient UI state separate from the provided model
2 participants