[Issue 683] - when changing columns displayed, grid will only make on… #982
Conversation
|
Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: c8d1363 (Please note that this is a fully automated comment.) |
Codecov Report
@@ Coverage Diff @@
## master #982 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 307 308 +1
Lines 5573 5592 +19
Branches 701 704 +3
=====================================
+ Hits 5573 5592 +19
Continue to review full report at Codecov.
|
|
Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: 618b093 (Please note that this is a fully automated comment.) |
|
Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: 7e0ad29 (Please note that this is a fully automated comment.) |
|
@Blackbaud-LuisBello Is the intended behavior that only one call will be made to the (Also, the Plunkr isn't working anymore on the issue, so I may be reproducing the List View Grid incorrectly. #683) |
|
the behavior should be that the main grid makes only one call on a column upgrade. the column selector modal is its own list and so, it's also making a data provider call which you might be seeing as that "second" one. The best way to test this fix is using a grid hooked up to a data service to fetch its data. when changing columns, only one service call should be made to fetch the new data. We had this same bug in our project and with this fix, our grid now correctly only makes one data call |
Blackbaud-LuisBello
force-pushed
the
grid-data-provider-fix
branch
from
7e0ad29 to
2ae1212
Compare
| this.next(new ListSearchSetOptionsAction( | ||
| new ListSearchSetSearchTextAction(searchText), | ||
| new ListSearchSetFieldSelectorsAction(fieldSelectors), | ||
| new ListSearchSetFunctionsAction(sortFunctions) |
There was a problem hiding this comment.
You have fieldSelectors as the last parameter of searchSetOptions(), but it's the middle parameter for ListSearchOptionsAction constructor. Possible to make them consistent?
UPDATE: This shouldn't be an issue when you write the "options" setter's interface (see previous comment).
|
Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: 2ae1212 (Please note that this is a fully automated comment.) |
| undefined, | ||
| setFunctions, | ||
| displayedColumns.map(d => d.field) | ||
| ); |
There was a problem hiding this comment.
I like the convenience of having a single method for all options, but I can imagine this getting a bit unwieldy, especially if a contributor needs to remember in which order the arguments need to be called.
I recommend that we either maintain the individual setter methods, or create a new interface that would allow a single key-value parameter (see example below). I prefer the individual setter methods, personally, because it reduces the complexity of having yet another interface on this thing.
@Blackbaud-BobbyEarl: Thoughts?
export ListSearchSetOptionsConfig {
searchText: string;
sortFunctions: ((data: any, searchText: string) => boolean)[];
fieldSelectors: Array<string>;
}
// ...
public searchSetOptions(options: ListSearchSetOptionsConfig) { }
There was a problem hiding this comment.
I agree with @Blackbaud-SteveBrush that if you're going to combine the methods into a single operation, it's argument should be of a new type.
I'm mostly impartial on multiple setters vs using a single method with a known argument type. If forced, I would probably lean towards the latter. The former seems like it might be possible to leak implementation details, do I need to call the methods in a specific order, for example.
There was a problem hiding this comment.
You bring up a good point, @Blackbaud-BobbyEarl.
@Blackbaud-LuisBello: If you wouldn't mind generating the interface for the "options" setter, that would be ideal!
There was a problem hiding this comment.
sure thing, on it!
There was a problem hiding this comment.
It seems like just some extra overhead for little benefit. Since you can just look at the function definition which is a common task for developers (and quite trivial with IDEs).
…e service call to update data unit tests passing, 100% coverage. No changes to visual component were made.
Blackbaud-LuisBello
force-pushed
the
grid-data-provider-fix
branch
from
2ae1212 to
7e58167
Compare
|
@Blackbaud-SteveBrush @Blackbaud-BobbyEarl updated to use the ListSearchModel instead of separate parameters, should be good now |
|
Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: 7e58167 (Please note that this is a fully automated comment.) |
…e service call to update data
unit tests passing, 100% coverage. No changes to visual component were made.
Addresses: #683