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/layers #189

Merged
merged 4 commits into from Mar 21, 2019

Conversation

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

commented Mar 21, 2019

No description provided.

@georgios-uber georgios-uber added this to the Nebula 1.0.0 milestone Mar 21, 2019

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

@georgios-uber georgios-uber requested review from supersonicclay and ibgreen Mar 21, 2019

export { ArrowStyles, DEFAULT_ARROWS, MAX_ARROWS } from './style.js';

// Layers
export { default as EditableGeoJsonLayer } from './layers/editable-geojson-layer.js';

This comment has been minimized.

Copy link
@supersonicclay

supersonicclay Mar 21, 2019

Collaborator

So, will both of these work after this PR?

const EditableGeoJsonLayer = require('nebula.gl')
const EditableGeoJsonLayer = require('@nebula.gl/layers')

This comment has been minimized.

Copy link
@georgios-uber

georgios-uber Mar 21, 2019

Author Collaborator

That is the idea 👍
Similar to deck.gl and @deck.gl/core

This comment has been minimized.

Copy link
@georgios-uber

georgios-uber Mar 21, 2019

Author Collaborator

To be precise you will need to:

const { EditableGeoJsonLayer } = require('nebula.gl');
const { EditableGeoJsonLayer } = require('@nebula.gl/layers');

or

import { EditableGeoJsonLayer } from 'nebula.gl';
import { EditableGeoJsonLayer } from '@nebula.gl/layers';

@georgios-uber georgios-uber merged commit aa57384 into master Mar 21, 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 create-layers branch Mar 21, 2019

@ibgreen
Copy link
Collaborator

left a comment

This looks great, it aligns very nicely with the module strategy in other vis.gl repos!

One thing missing here that I think is quite important is to document the module structure. Explain to users (and developers) what modules are available, what they contain etc, including clearly calling out that the nebula.gl module is a deprecated module included for backwards compatibility.

The docs should likely also be lightly reorganized to reflect a per-module structure.

On a separate note, I would suggest not using the directory name core for the deprecated module.

The reason is that most of our vis.gl monorepos have a (non-react) core module (@luma.gl/core, @deck.gl/core, loaders.gl/core). If you ever need a set of common classes you would put them there. Even if not, just for the sake of alignment it is better to not confuse devs that move between repos by using core for different things.

FWIW, we put our deprecated unscoped modules (luma.gl/deck.gl/loaders.gl) in the modules/main directory in each repo...

Finally, a nit: I see lots of commented code left in the example, recommend cleaning up asap.

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.