fix: initial checkbox filtering when using view toggle model #547
Conversation
Codecov Report
@@ Coverage Diff @@
## main #547 +/- ##
==========================================
- Coverage 85.61% 85.57% -0.04%
==========================================
Files 759 759
Lines 15554 15560 +6
Branches 1985 1987 +2
==========================================
Hits 13316 13316
- Misses 2205 2211 +6
Partials 33 33
Continue to review full report at Codecov.
|
This comment has been minimized.
This comment has been minimized.
| value: viewOption | ||
| })); | ||
|
|
||
| this.populateStaticControls(); |
There was a problem hiding this comment.
this delineation between static and dynamic controls seems like we're embedding too much knowledge of the controls into the renderer.
There was a problem hiding this comment.
Yeah, open to suggestions then. Preferably without going down a rabbit hole. :-)
There was a problem hiding this comment.
of course, of course. When using on init to set the delegate, does this need go away?
There was a problem hiding this comment.
Hmm. Not really since there are still some controls that fetch data and some that don't.
We could just always repopulate all controls on fetch so the renderer doesn't have to have knowledge of which filters are static vs dynamic, but that seems worse. Not sure how else to handle this since we kind of mix and match how we provide data to our control filters. I kind of think they should all just use the modelAPI to getData(), even if its just an observable that returns a static list.
There was a problem hiding this comment.
We could just always repopulate all controls on fetch so the renderer doesn't have to have knowledge of which filters are static vs dynamic, but that seems worse
Worse in that its an extra control render that's not necessary? If it's not noticeable, that seems preferable to me (not sure if that would cause things like losing control state on refresh, though).
I kind of think they should all just use the modelAPI to getData(), even if its just an observable that returns a static list.
Agreed, that way the caller doesn't know/care how the callee is implemented. Is this different than the "repopulate all on fetch" above?
There was a problem hiding this comment.
Yeah, okay. I think we agree. The populate on fetch does seem the least bad. I'll make that change.
|
|
||
| private populateStaticControls(): void { | ||
| /* | ||
| * CAUTION: If TableWidgetViewToggleModel is used, this.model is not hydrated until after setView() is called, |
There was a problem hiding this comment.
this.model should always be hydrated (first line of the constructor). It sounds like the issue is the view toggle isn't picking a delegate, by default, and forcing the renderer to do so?
There was a problem hiding this comment.
Correct. That's exactly what it is, but not sure how to default one in the model without a constructor or some other lifecycle type method. Just assigning one on the class property doesn't work.
There was a problem hiding this comment.
sounds like a case for ModelOnInit!
There was a problem hiding this comment.
Oh, lovely. Didn't know we had that. Looks like the fine gentleman who built the dashboarding system thought of about everything.
There was a problem hiding this comment.
you'd be amazed by all the cool features we have sitting on a github shelf somewhere 😉
There was a problem hiding this comment.
I'm still waiting for the variable within a variable use case to come in handy.
This comment has been minimized.
This comment has been minimized.
Description
Fixes issue with initial checkbox filter not being applied to query due to table being wrapped in TableWidgetViewToggleModel. This is due to the the TableWidgetViewToggleModel requiring a view be set prior to it creating the delegate model.