-
Notifications
You must be signed in to change notification settings - Fork 16
Update Grid to work with the new abstraction of HierarchicalArrayUpdater #275
Update Grid to work with the new abstraction of HierarchicalArrayUpdater #275
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 4 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @denis-anisimov and @gilberto-torrezan)
src/main/java/com/vaadin/flow/component/treegrid/TreeGrid.java, line 143 at r1 (raw file):
return new TreeGridArrayUpdater()
Wouldn't it be better to extract this as a nested class ?
src/main/java/com/vaadin/flow/component/treegrid/TreeGrid.java, line 191 at r1 (raw file):
GridArrayUpdater arrayUpdater,
Doesn't it make sense to introduce ArrayUpdater
class as a parameter type to DataCommunicatorBuilder
so that this method accepts arrayUpdater
with a parameter type so that there is no need to check its type and throw an ISE exception below ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 4 files reviewed, 4 unresolved discussions (waiting on @denis-anisimov and @vaadin-bot)
src/main/java/com/vaadin/flow/component/treegrid/TreeGrid.java, line 143 at r1 (raw file):
Previously, denis-anisimov (Denis) wrote…
return new TreeGridArrayUpdater()
Wouldn't it be better to extract this as a nested class ?
Done.
src/main/java/com/vaadin/flow/component/treegrid/TreeGrid.java, line 191 at r1 (raw file):
Previously, denis-anisimov (Denis) wrote…
GridArrayUpdater arrayUpdater,
Doesn't it make sense to introduce
ArrayUpdater
class as a parameter type toDataCommunicatorBuilder
so that this method acceptsarrayUpdater
with a parameter type so that there is no need to check its type and throw an ISE exception below ?
Done.
d6da197
to
af63e18
Compare
SonarQube analysis reported 24 issues Top 10 extra issuesNote: The following issues were found on lines that were not modified in the pull request. Because these issues can't be reported as line comments, they are summarized here:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r2.
Reviewable status: complete! all files reviewed, all discussions resolved
Depends on vaadin/flow#4427
This change is