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

Create @nebula.gl/overlays #182

Merged
merged 1 commit into from Mar 18, 2019

Conversation

Projects
None yet
3 participants
@georgios-uber
Copy link
Collaborator

georgios-uber commented Mar 14, 2019

No description provided.

@georgios-uber georgios-uber self-assigned this Mar 14, 2019

@georgios-uber

This comment has been minimized.

Copy link
Collaborator Author

georgios-uber commented Mar 14, 2019

Seems like this approach has performance issues. @ibgreen please help debug.

@georgios-uber georgios-uber force-pushed the new-overlay branch from 9b1cdf6 to 79d6e8b Mar 14, 2019

@georgios-uber

This comment has been minimized.

Copy link
Collaborator Author

georgios-uber commented Mar 14, 2019

Fixed a bunch of issues. Should work now.

@georgios-uber

This comment has been minimized.

Copy link
Collaborator Author

georgios-uber commented Mar 14, 2019

@ibgreen
Copy link
Collaborator

ibgreen left a comment

Assume doc examples need to be updated to show the new import path?

E.g. from HtmlClusterOverlay doc which looks out of date?

<Nebula
  ref={nebula => (this.nebula = nebula || this.nebula)}
  {...{ layers, viewport }}
>
  <YourNewClass />
</Nebula>

Change to?

import {HTMLClusterOverlay} from '@nebula.gl/overlays';
<DeckGL layers={} ...>
  <HTMLClusterOverlay ... />
</Nebula>
'nebula.gl-react': REACT_SRC_DIR,

'@nebula.gl/overlays/dist': OVERLAYS_SRC_DIR,

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 14, 2019

Collaborator
  • Why do you need a sepate alias to dist? In other repos, we switch the alias between src and dist to enable testing from source or compiled code.
  • I guess there should be an alias for each module?
  • We often have an aliases.js file at the root listing the aliases that we just import here and in other places.

This comment has been minimized.

Copy link
@georgios-uber

georgios-uber Mar 18, 2019

Author Collaborator

More help on how to do this?

Show resolved Hide resolved modules/overlays/test/overlays/html-tooltip-overlay.test.js Outdated
import Renderer from 'react-test-renderer';

import { viewport } from '../../../../core/src/test/mocks';
import Nebula from '../../lib/nebula-react';

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 14, 2019

Collaborator

Tip: We import aliases.js in test files (and set them with module-alias) so that we can import using the right paths.

"main": "dist/index.js",
"module": "dist-es6/index.js",
"files": [
"dist",

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 14, 2019

Collaborator

Maybe switch to single dist folder (dist/es5 / dist/es6)

Show resolved Hide resolved ...s/overlays/test/overlays/__snapshots__/html-tooltip-overlay.test.js.snap Outdated
Show resolved Hide resolved examples/overlays/package.json Outdated
Show resolved Hide resolved examples/overlays/package.json Outdated

@georgios-uber georgios-uber force-pushed the new-overlay branch from 79d6e8b to 4a3d33d Mar 14, 2019

@georgios-uber georgios-uber force-pushed the new-overlay branch 2 times, most recently from 98b4da4 to 9098875 Mar 18, 2019

@georgios-uber georgios-uber requested a review from Pessimistress Mar 18, 2019

@georgios-uber georgios-uber force-pushed the new-overlay branch from 9098875 to effc109 Mar 18, 2019

@georgios-uber georgios-uber merged commit 178e212 into master Mar 18, 2019

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@georgios-uber georgios-uber deleted the new-overlay branch Mar 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.