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

Warn user if required sprite property is not present or non-existent image is requested from sprite #6831

Closed
wants to merge 4 commits into from

Conversation

ryanhamley
Copy link
Contributor

@ryanhamley ryanhamley commented Jun 18, 2018

Launch Checklist

  • briefly describe the changes in this PR
    • logs a console warning if sprite URL is not defined and a property that requires sprites is used
    • logs a console warning if an image not in the sprite is requested
  • write tests for all new functionality
    • i don't see tests for the existing glyph validation. do we think unit tests are necessary for this? if so, i can add them for both sprite and glyph validation
  • manually test the debug page
    screen shot 2018-11-01 at 5 42 27 pm
    screen shot 2018-11-01 at 5 48 21 pm

Closes #6823

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.

nice! the style-spec validation tests are a little different from our other unit tests, and use fixtures.
here are the relevant fixtures for the missing glyphs validation:
https://github.com/mapbox/mapbox-gl-js/blob/master/test/unit/style-spec/fixture/missing-glyphs.input.json
https://github.com/mapbox/mapbox-gl-js/blob/master/test/unit/style-spec/fixture/missing-glyphs.output.json

we also want to be careful and make sure this error doesn't fully break existing styles that fail this validation check. from what I can tell right now it does block rendering if the validation fails on the initial style load, but it doesn't break if the validation fails with addLayer or set*Property

@ryanhamley ryanhamley force-pushed the 6823-validate-sprite-property branch 3 times, most recently from 8fc2c97 to b5acbb5 Compare November 2, 2018 00:29
@ryanhamley ryanhamley changed the title Throw ValidationError if required sprite property is not present Warn user if required sprite property is not present or non-existent image is requested from sprite Nov 2, 2018
@ryanhamley
Copy link
Contributor Author

Closing in favor of #7987

@ryanhamley ryanhamley closed this Mar 4, 2019
@ryanhamley ryanhamley deleted the 6823-validate-sprite-property branch March 4, 2019 20:16
@ansis
Copy link
Contributor

ansis commented Mar 4, 2019

@ryanhamley I wasn't aware of this pr, sorry. Do you think it makes sense to still add the warning for missing icons to the other pr? (in the case the user doesn't add the missing icon in the event's callback)

@ryanhamley
Copy link
Contributor Author

No problem. It's something I quickly took a stab at awhile ago and then never went back to. The goal was always a more fleshed out option like this. A warning might be useful because right now you wouldn't necessarily know that anything was wrong if you weren't listening for the event. I can imagine spending a few hours tearing my hair out trying to figure out why my image wasn't showing up only to realize there's a typo or something.

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.

3 participants