-
Notifications
You must be signed in to change notification settings - Fork 13.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(deck.gl): update view state on property changes (#17720) #17826
Conversation
Codecov Report
@@ Coverage Diff @@
## master #17826 +/- ##
==========================================
- Coverage 67.07% 67.07% -0.01%
==========================================
Files 1609 1609
Lines 64899 64901 +2
Branches 6866 6867 +1
==========================================
Hits 43533 43533
- Misses 19500 19502 +2
Partials 1866 1866
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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.
Thanks for the fix! Works well, but minor performance improvement suggestion
@@ -64,6 +64,12 @@ export class DeckGLContainer extends React.Component { | |||
}; | |||
} | |||
|
|||
UNSAFE_componentWillReceiveProps(nextProps) { | |||
if (nextProps.viewport !== this.props.viewport) { |
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.
I believe this will cause unnecessary rerenders as the object reference may change despite its properties being unchanged. Let's use lodash
here, something like
import { isEqual } from 'lodash';
if (!isEqual(nextProps.viewport, this.props.viewport)) {
...
},
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.
Thanks for the hint. Is adressed by last commit (though checks failed again)...
@hbruch can you rebase this? I believe there may have been some CI issues when you pushed these last changes |
093cf9d
to
dbbfec7
Compare
@villebro Is there anything I could do to have this merged? Thx |
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.
Oh, sorry about the delay!
SUMMARY
Updates viewport state of deck.gl on filter updates
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before (screeshot 1 & 2):
After:
update_viewport_480.mov
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION