Make ZoneGridModel more forgiving (handle possibly out of sync state) #3530
Conversation
There was a problem hiding this comment.
Thanks for catching and taking a look at this issues Don!
It's certainly clearly problematic if this thing throws because saved state is no longer valid! I also like how you are resolving back to the default state to get back to a good state 👍 But I realised that assumes the default state respects the constraints, and parseMappings() is used both to set up the initial default state and to handle changing the state.
I wonder if we should only be this lenient in the latter case? It seems wrong to allow apps to set up a Grid where the defaults are always wrong - especially if we expect loading the defaults to resolve the issue. Maybe we could add a second strict parameter to parseMappings(), and conditionally throw or warn based on that?
cmp/zoneGrid/ZoneGridModel.ts
Outdated
| remove(mapping, it => offenders.includes(it.field)); | ||
| } | ||
|
|
||
| if ((zone == 'tl' || zone == 'tr') && isEmpty(mapping)) { |
There was a problem hiding this comment.
Great catch! But I think we want to do this outside of the !isEmpty(limit.only) condition.
(I know enforcing only is currently the only place where we remove mappings, but I think the intent is clearer if we always do this as the final check of parseMappings)
There was a problem hiding this comment.
Great note Tom. I considered the same, but apparently made the wrong choice ;)
cmp/zoneGrid/ZoneGridModel.ts
Outdated
| }); | ||
|
|
||
| if (!isEmpty(offenders)) { | ||
| remove(mapping, it => offenders.includes(it.field)); |
There was a problem hiding this comment.
set to default again here. Only need to warn for one field and say we're going to the default immediately at that point.
… state)
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.