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

add type for defaultProps in path outline layer #2521

Merged
merged 6 commits into from
Jan 29, 2019

Conversation

XiaokaiUber
Copy link
Contributor

@XiaokaiUber XiaokaiUber commented Dec 19, 2018

For #2477

Background

In path outline layer, the defaultProps don't have types specified. We need to specify type for those props.

Change List

  • Add type to the defaultProps in path outline layer

note: tested with render test and layer-browser. compared the rendering result with that in 6.3-release

@XiaokaiUber XiaokaiUber force-pushed the update-props-path-outline-layer branch from 45e1498 to 4f135d5 Compare December 19, 2018 22:28
@Pessimistress
Copy link
Collaborator

Test your change in the layer browser.

@XiaokaiUber XiaokaiUber force-pushed the update-props-path-outline-layer branch from 8c908fb to abb1fc8 Compare December 31, 2018 19:16
@XiaokaiUber XiaokaiUber force-pushed the update-props-path-outline-layer branch from abb1fc8 to d4ac548 Compare January 28, 2019 07:21
@Pessimistress
Copy link
Collaborator

Did you test this layer with getZLevel set to 0?

@XiaokaiUber
Copy link
Contributor Author

Did you test this layer with getZLevel set to 0?

Yes. If 0 or any number is passed in, the image shows up but there is no outline. This is the same as that in 6.2-release. But in 6.3-release, it will crash.

@XiaokaiUber XiaokaiUber force-pushed the update-props-path-outline-layer branch from d4ac548 to 2450c14 Compare January 29, 2019 00:45
@@ -2,6 +2,7 @@ import {PathLayer} from '@deck.gl/layers';
import GL from '@luma.gl/constants';
import {Framebuffer, Texture2D} from 'luma.gl';
import outline from '../shaderlib/outline/outline';
import assert from '../../../core/src/utils/assert';
Copy link
Collaborator

Choose a reason for hiding this comment

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

You cannot import something from another module. This is not going to be the directory structure when the module is published.

getValue: (object, index) => getZLevel(object, index) || 0
getValue: (object, index) => {
const zLevel = getZLevel(object, index);
assert(!isNaN(zLevel), '`getZLevel` does not return a number!');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just keep the old behavior before your change.

@XiaokaiUber XiaokaiUber merged commit c86afee into master Jan 29, 2019
@XiaokaiUber XiaokaiUber deleted the update-props-path-outline-layer branch January 29, 2019 17:26
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