-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
[Chore] Remove default props and react-onclickoutside in react functional components #2679
Conversation
Signed-off-by: Shan He <heshan0131@gmail.com>
Signed-off-by: Shan He <heshan0131@gmail.com>
✅ Deploy Preview for keplergl2 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Signed-off-by: Shan He <heshan0131@gmail.com>
Signed-off-by: Shan He <heshan0131@gmail.com>
Signed-off-by: Shan He <heshan0131@gmail.com>
Signed-off-by: Shan He <heshan0131@gmail.com>
Signed-off-by: Shan He <heshan0131@gmail.com>
Signed-off-by: Shan He <heshan0131@gmail.com>
@@ -59,7 +59,7 @@ const config = { | |||
// automatically injected kepler.gl package version into the bundle | |||
replace({ | |||
__PACKAGE_VERSION__: KeplerPackage.version, | |||
exclude: /node_modules/ | |||
include: /constants\/src\/default-settings\.ts/ |
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.
this will fix the child_process not found error from loaders.gl
@@ -0,0 +1,32 @@ | |||
// Copyright 2022 Foursquare Labs, Inc. All Rights Reserved. | |||
|
|||
import document from 'global/document'; |
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.
remove react-onclickoutside, implements a simple hooks to handle click outside
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.
Looks good. I hope it won't introduce too many conflicts for our upstreaming.
} | ||
] | ||
}; | ||
LoadDataModal.defaultLoadingMethods = [ |
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.
Do we need to duplicate loading methods? defaultLoadingMethods is same as above.
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.
We still need this so that the custom LoadDataModal can access this via LoadDataModal.defaultLoadingMethods
https://github.com/keplergl/kepler.gl/pull/2679/files#r1794197038
But we don't need ti duplicated declaration in 2 places
...component, | ||
component: SIDEBAR_COMPONENTS[component.id], | ||
iconComponent: SIDEBAR_ICONS[component.id] | ||
})); | ||
|
||
const getCustomPanelProps = get(CustomPanels, ['defaultProps', 'getProps']) || (() => ({})); | ||
const fullPanels = [...defaultSidePanels, ...(CustomPanels.panels || [])]; |
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.
Do we need CustomPanels to later be assigned to defaultPanels ?https://github.com/keplergl/kepler.gl/pull/2679/files#diff-f9335fee0a1e5c33d813c5d5705ed9a34cae228fb069c72feccacd4e17d138a8R247
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 think we are accessing Sidepanel.defaultProps.panels
in studio, now the property can be accessed at SidePanel.defaultPanels
|
||
export default withTheme(onClickOutside(CustomPicker) as React.ComponentType<CustomPickerProps>); | ||
export default withTheme(CustomPicker as React.FC<CustomPickerProps>); |
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.
We don't need to cast here.
Not that much, most of them can be fixed by following the type errors |
@@ -11,7 +11,7 @@ import {loadRemoteMap, loadSample, loadSampleConfigurations} from '../actions'; | |||
|
|||
const CustomLoadDataModalFactory = (...deps) => { | |||
const LoadDataModal = LoadDataModalFactory(...deps); | |||
const defaultLoadingMethods = LoadDataModal.defaultProps.loadingMethods; | |||
const defaultLoadingMethods = LoadDataModal.defaultLoadingMethods; |
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.
here we can access the default loading methods
Signed-off-by: Shan He <heshan0131@gmail.com>
Use of defaultProps and findDomNode has been deprecated. This change tries to remove use of both, to avoid React warnings in the dev console
Still seeing warnings about deprecated APIs from the following dependencies, further dependency upgrade is needed