Skip to content
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

Remove kepler.gl recursive includes #51

Merged
merged 1 commit into from
Dec 7, 2020

Conversation

unconed
Copy link
Contributor

@unconed unconed commented Dec 7, 2020

  • Remove kepler.gl/ includes from react components entirely
    • Replace DIMENSIONS from kepler.gl/constants with the single constant we use. This is fixed in kepler.gl anyway.
    • Move ReactIntl integration to kepler-integration example to remove kepler.gl/localization dependency from component. This context is already provided inside kepler.gl.
  • Move luma.gl to a peer dependency of the sub-packages to avoid version conflicts with examples
  • Move react-intl dep to top level to avoid duplicated intl context instances in example
  • Pin more luma/deck versions to 8.2.0 to match kepler

- Remove kepler.gl/ includes from react components entirely
- Move ReactIntl integration to kepler-integration example as kepler.js provides its own already
- Move react-intl dep to top level to avoid duplicated context instances
<div style={{transition: 'margin 1s, height 1s', flex: 1}}>
<AutoSizer>
{({height, width}) => (
<KeplerGl
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kepler would internally provide it's own IntlProvider. I guess these don't conflict (or kepler overrides the incoming object?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah the inner one clobbers the outer one, so this should be fine. It's mainly there to make the modal work even though it's not embedded inside Kepler itself.

@@ -70,7 +70,7 @@ class ExportVideoModal extends Component {
content: {
top: 'auto',
left: 'auto',
right: `calc(50% - ${DIMENSIONS.sidePanel.width / 2}px)`,
right: `calc(50% - ${SIDEPANEL_WIDTH / 2}px)`,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see how this is necessary to import. If kepler's styled-components theme provides this we could pull it from there, but I suppose even that is unnecessary for this usage.

Copy link
Contributor Author

@unconed unconed Dec 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think export-video-modal.js is mainly useful for demonstration. It's not used with a custom modal rendering ExportVideoContainer directly. So it's not critical that this value can't be edited.

@chrisgervang chrisgervang merged commit 753b7d0 into visgl:master Dec 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants