Add GroupingChooserModel.setDimensions()
#3743
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Ryanseanlee
commented
Jul 31, 2024
amcclain
reviewed
Aug 1, 2024
cmp/grouping/GroupingChooserModel.ts
Outdated
| @@ -308,7 +319,7 @@ export class GroupingChooserModel extends HoistModel { | |||
| return sortBy( | |||
| this.favorites.map(value => ({ | |||
| value, | |||
| label: this.getValueLabel(value) | |||
| label: this.getValueLabel(value) ?? value | |||
There was a problem hiding this comment.
Why not have getValueLabel handle the edge case ?
+ Early-out if all dims remained in place
+ Make dimensionNames private - did not look like this was anything that needed to be a public property, ensures this stays in sync with dimensions, only settable from within `setDimensions()`
amcclain
reviewed
Aug 5, 2024
amcclain
changed the title
GroupingChooserModel.setDimensions()
haynesjm42
reviewed
Aug 9, 2024
haynesjm42
approved these changes
Aug 30, 2024
There was a problem hiding this comment.
Tested this out for my use case and worked perfectly.
Am now able to create the GroupingChooserModel as a readonly member of my model with no dimensions, and I can set the dimensions when I load initial state in my model or when the relevant state changes to determine the dimensions - no more re-creating models and much less/simpler code!
|
@Ryanseanlee I think we just need a changelog entry and this can be merged |
|
Great thanks much - I'll add that in now and merge it. 🚀 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.