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

[Maps] Load Maki icons from spritesheet #42499

Merged
merged 10 commits into from
Aug 6, 2019

Conversation

nickpeihl
Copy link
Member

@nickpeihl nickpeihl commented Aug 1, 2019

This PR enables the use of @elastic/maki icons from a spritesheet. The spritesheet contains PNG icons generated from SVGs using a signed distance field algorithm.

This relies on elastic/maki#3 which creates the spritesheet. We should review and merge that PR first and create an @elastic/maki npm package.

@elastic/maki npm package also includes arrow-es, boat-es, and,car-top-es icons that will work great for directional pointing

TODO

  • Update and write tests
  • Finalize @elastic/maki API and merge the PR
  • Publish the @elastic/maki npm package
  • Update the @elastic/maki dependency in package.json

@thomasneirynck
Copy link
Contributor

Finding this on Kibana startup

Cannot read property 'forEach' of undefined
Version: 8.0.0
Build: 9007199254740991
TypeError: Cannot read property 'forEach' of undefined
    at Object../x-pack/legacy/plugins/maps/public/layers/styles/symbol_utils.js (http://localhost:5601/nuz/bundles/commons.bundle.js:257171:24)
    at __webpack_require__ (http://localhost:5601/nuz/bundles/kibana.bundle.js:83:30)
    at Object../x-pack/legacy/plugins/maps/public/layers/styles/components/vector/legend/symbol_icon.js (http://localhost:5601/nuz/bundles/commons.bundle.js:255466:21)
    at __webpack_require__ (http://localhost:5601/nuz/bundles/kibana.bundle.js:83:30)
    at Object../x-pack/legacy/plugins/maps/public/layers/styles/components/vector/vector_style_symbol_editor.js (http://localhost:5601/nuz/bundles/commons.bundle.js:256838:20)
    at __webpack_require__ (http://localhost:5601/nuz/bundles/kibana.bundle.js:83:30)
    at Object../x-pack/legacy/plugins/maps/public/layers/styles/components/vector/vector_style_editor.js (http://localhost:5601/nuz/bundles/commons.bundle.js:256427:35)
    at __webpack_require__ (http://localhost:5601/nuz/bundles/kibana.bundle.js:83:30)
    at Object../x-pack/legacy/plugins/maps/public/layers/styles/vector_style.js (http://localhost:5601/nuz/bundles/commons.bundle.js:257401:28)
    at __webpack_require__ (http://localhost:5601/nuz/bundles/kibana.bundle.js:83:30)

https://github.com/thomasneirynck/kibana/blob/1b0eda0d29d3f4b1de94df1ffff76f4fd111047b/x-pack/legacy/plugins/maps/public/layers/styles/symbol_utils.js#L17

The maki object has a layouts and spritesheets property, but not an svgArray property.

Anyway to resolve?

@nickpeihl
Copy link
Member Author

Finding this on Kibana startup

Cannot read property 'forEach' of undefined
Version: 8.0.0
Build: 9007199254740991
TypeError: Cannot read property 'forEach' of undefined
    at Object../x-pack/legacy/plugins/maps/public/layers/styles/symbol_utils.js (http://localhost:5601/nuz/bundles/commons.bundle.js:257171:24)
    at __webpack_require__ (http://localhost:5601/nuz/bundles/kibana.bundle.js:83:30)
    at Object../x-pack/legacy/plugins/maps/public/layers/styles/components/vector/legend/symbol_icon.js (http://localhost:5601/nuz/bundles/commons.bundle.js:255466:21)
    at __webpack_require__ (http://localhost:5601/nuz/bundles/kibana.bundle.js:83:30)
    at Object../x-pack/legacy/plugins/maps/public/layers/styles/components/vector/vector_style_symbol_editor.js (http://localhost:5601/nuz/bundles/commons.bundle.js:256838:20)
    at __webpack_require__ (http://localhost:5601/nuz/bundles/kibana.bundle.js:83:30)
    at Object../x-pack/legacy/plugins/maps/public/layers/styles/components/vector/vector_style_editor.js (http://localhost:5601/nuz/bundles/commons.bundle.js:256427:35)
    at __webpack_require__ (http://localhost:5601/nuz/bundles/kibana.bundle.js:83:30)
    at Object../x-pack/legacy/plugins/maps/public/layers/styles/vector_style.js (http://localhost:5601/nuz/bundles/commons.bundle.js:257401:28)
    at __webpack_require__ (http://localhost:5601/nuz/bundles/kibana.bundle.js:83:30)

https://github.com/thomasneirynck/kibana/blob/1b0eda0d29d3f4b1de94df1ffff76f4fd111047b/x-pack/legacy/plugins/maps/public/layers/styles/symbol_utils.js#L17

The maki object has a layouts and spritesheets property, but not an svgArray property.

Anyway to resolve?

I needed to update yarn.lock. This should be fixed now.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

Thanks for switching the icon loading. This is great.

Can you also delete the server end point logic for /maps/sprite/maki along with the constants SPRITE_PATH and MAKI_SPRITE_PATH since they are no longer used

import _ from 'lodash';
import assert from 'assert';

function createImage(image, { width, height }, channels, data) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move this map box code into its own file to keep isolated?

return a.href;
}
import { spritesheet } from '@elastic/maki';
import sprites from '@elastic/maki/dist/sprite@1.png';
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way to see if the device is retina display and load @2.png if it is?

@nickpeihl nickpeihl marked this pull request as ready for review August 6, 2019 17:22
Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

This is great. Love the new arrow, boat and car-top icons. Thanks for getting all of this done

lgtm
code review, tested in chrome and firefox

@nickpeihl nickpeihl added the [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation label Aug 6, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-gis

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@nickpeihl nickpeihl merged commit 8a3b96d into elastic:master Aug 6, 2019
nickpeihl added a commit to nickpeihl/kibana that referenced this pull request Aug 6, 2019
@nickpeihl nickpeihl deleted the sdf-maki-icons branch August 6, 2019 21:06
nickpeihl added a commit that referenced this pull request Aug 6, 2019
@thomasneirynck thomasneirynck mentioned this pull request Aug 7, 2019
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants