Skip to content

move tile map visualization out of vislib#9441

Merged
ppisljar merged 3 commits intoelastic:masterfrom
ppisljar:vislib/extractMaps
Dec 26, 2016
Merged

move tile map visualization out of vislib#9441
ppisljar merged 3 commits intoelastic:masterfrom
ppisljar:vislib/extractMaps

Conversation

@ppisljar
Copy link
Copy Markdown
Contributor

@ppisljar ppisljar commented Dec 10, 2016

moves tilemap visualization out of vislib

  • tile map has nothing to do with vislib (which is d3 charts implementation). If we move it outside of vislib it will allow us to move faster with vislib and tile map development.
  • common components were extracted out as well and are now part of Vis. They should be there for all visualizations to use. This way all visualizations can use same tooltip and color provider and we can achieve consistency across application even with 3rd party plugins. (we saw plugins importing colors directly from vislib which is not a good practice). Over time other common components will be extracted (like label provider, data provider, resize checker, event dispatcher, error handler and alert notifier)

@ppisljar ppisljar added Feature:Visualizations Generic visualization features (in case no more specific feature label is available) review v5.2.0 v6.0.0 labels Dec 10, 2016
@ppisljar ppisljar force-pushed the vislib/extractMaps branch 2 times, most recently from de10e9f to 6ee1ba8 Compare December 10, 2016 19:43
@ppisljar ppisljar mentioned this pull request Dec 12, 2016
Merged
@spalger
Copy link
Copy Markdown
Contributor

spalger commented Dec 13, 2016

Can we create a separate PR for the extraction of the "components" done in this pr?

I also detect a fair amount of copy and paste, is this intended to be a PR that leads to more changes to establish tilemap as it's own visualization type? Removing the dispatch/data/layout classes and converting it to be a template based vis?

@spalger spalger self-assigned this Dec 13, 2016
@ppisljar
Copy link
Copy Markdown
Contributor Author

ppisljar commented Dec 13, 2016

@spalger they are two separate commits, does that work for you ?

  • moving out components is just moving colors/ and tooltips/ fodlers from vislib/components to vis/components and updating every reverence to ui/vislib/components/color (or tooltip) to ui/vis/components/color (or tooltip)

  • moving maps out of vislib

  • maps were actually just taken out together with simplified version of dispatch, layout, data and config classes, to make sure this PR doesn't have any impact on functionality or maps implementation. Thomas will take that further, as he has more ideas what to do with the maps (and probably move away from dispatch and layout ....) so yes, mostly its copy pasted parts from vislib implementation.
    [ _chart.js, data.js, layout.js, maps_config.js, dispatch.js, maps_renderbot.js, maps_vis_type.js] and maps.js is based on vis.js and handler.js

@ppisljar
Copy link
Copy Markdown
Contributor Author

ppisljar commented Dec 13, 2016

sorry i forgot to force push the update to 2 commits ... its there now

@ppisljar
Copy link
Copy Markdown
Contributor Author

ppisljar commented Dec 13, 2016

wow .... somehow i managed to break this while rebasing .... i will take a look at this tomorrow.

@thomasneirynck thomasneirynck self-assigned this Dec 13, 2016
@thomasneirynck thomasneirynck mentioned this pull request Dec 13, 2016
Closed
@ppisljar ppisljar force-pushed the vislib/extractMaps branch 2 times, most recently from 51ae1a7 to bb14e71 Compare December 14, 2016 09:48
@ppisljar
Copy link
Copy Markdown
Contributor Author

ppisljar commented Dec 14, 2016

jenkins, test this

@thomasneirynck thomasneirynck mentioned this pull request Dec 16, 2016
Merged
2 tasks
spalger
spalger approved these changes Dec 19, 2016
View reviewed changes
Copy link
Copy Markdown
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

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

LGMT, best I can tell the code works as expected and the front-end looks good

thomasneirynck
thomasneirynck approved these changes Dec 26, 2016
View reviewed changes
Copy link
Copy Markdown
Contributor

@thomasneirynck thomasneirynck left a comment

Choose a reason for hiding this comment

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

tested. works as before!

@ppisljar ppisljar merged commit ac35cdc into elastic:master Dec 26, 2016
@elastic-jasper elastic-jasper mentioned this pull request Dec 26, 2016
Merged
ppisljar added a commit that referenced this pull request Dec 26, 2016
@ppisljar
move tile map visualization out of vislib
418f67a 
Backports PR #9441

**Commit 1:**
moving colors and tooltips out of vislib to vis

* Original sha: fd1bc58
* Authored by ppisljar <peter.pisljar@gmail.com> on 2016-12-13T12:40:08Z

**Commit 2:**
moving tile map vis out of vislib

* Original sha: 126f1de
* Authored by ppisljar <peter.pisljar@gmail.com> on 2016-12-13T12:40:49Z

**Commit 3:**
rebasing on master and fixing linting

* Original sha: c665d4c
* Authored by ppisljar <peter.pisljar@gmail.com> on 2016-12-14T09:51:18Z
thomasneirynck pushed a commit that referenced this pull request Dec 26, 2016
@elastic-jasper @thomasneirynck
move tile map visualization out of vislib (#9639)
8e76bd8 
Backports PR #9441

**Commit 1:**
moving colors and tooltips out of vislib to vis

* Original sha: fd1bc58
* Authored by ppisljar <peter.pisljar@gmail.com> on 2016-12-13T12:40:08Z

**Commit 2:**
moving tile map vis out of vislib

* Original sha: 126f1de
* Authored by ppisljar <peter.pisljar@gmail.com> on 2016-12-13T12:40:49Z

**Commit 3:**
rebasing on master and fixing linting

* Original sha: c665d4c
* Authored by ppisljar <peter.pisljar@gmail.com> on 2016-12-14T09:51:18Z
@ppisljar ppisljar deleted the vislib/extractMaps branch December 26, 2016 18:22
@thomasneirynck thomasneirynck mentioned this pull request Jan 16, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@spalger spalger spalger approved these changes

@thomasneirynck thomasneirynck thomasneirynck approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@spalger spalger

@thomasneirynck thomasneirynck

Labels

Feature:Visualizations Generic visualization features (in case no more specific feature label is available) review v5.2.0 v6.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ppisljar @spalger @thomasneirynck