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.

Sign up for GitHub

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

Jump to bottom

move tile map visualization out of vislib #9441

Merged
merged 3 commits into from
Dec 26, 2016
Merged

move tile map visualization out of vislib #9441

merged 3 commits into from
Dec 26, 2016

Conversation

ppisljar
Copy link
Member

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
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
Member Author

@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
Member Author

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

@ppisljar
Copy link
Member Author

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
Member Author

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
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
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

@thomasneirynck thomasneirynck thomasneirynck approved these changes

@spalger spalger spalger approved these changes

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