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

check if before layer exists #5679

Merged
merged 4 commits into from
Nov 30, 2017

Conversation

jingsam
Copy link
Contributor

@jingsam jingsam commented Nov 15, 2017

I think we should check if before layer exists when move a layer

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page

@@ -622,6 +622,10 @@ class Style extends Evented {
this._order.splice(index, 1);

const newIndex = before ? this._order.indexOf(before) : this._order.length;
if (before && newIndex === -1) {
this.fire('error', { message: new Error(`Layer with id "${before}" does not exist on this map.`)});
Copy link
Member

Choose a reason for hiding this comment

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

The key here should be error since you're passing an error object, not a message.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see it was mixed up in the original code.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you're right, the key here should be error as well as here https://github.com/mapbox/mapbox-gl-js/pull/5679/files#diff-0fcf9ef37a1614185cad0dcbd5ec5529L571

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shall I change it to a {error: ....}?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes please @jingsam and it would be great if you could change the it to {error: new Error... on line 571 as well. thank you!

@mourner
Copy link
Member

mourner commented Nov 15, 2017

Otherwise looks good!

@andrewharvey
Copy link
Collaborator

@jingsam did you want to fix up the JSDoc just a few lines above your change while you're at it? https://github.com/mapbox/mapbox-gl-js/blob/master/src/style/style.js#L601

@jingsam
Copy link
Contributor Author

jingsam commented Nov 21, 2017

fixed tests. but this seems odd:

t.match(error.error, /does not exist on this map/);

I prefer error.message

Copy link
Contributor

@mollymerp mollymerp left a comment

Choose a reason for hiding this comment

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

thanks!!

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.

4 participants