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

Deprecate split chart option for tile map visualization #6001

Merged
merged 20 commits into from
Mar 29, 2016

Conversation

stormpython
Copy link
Contributor

Closes #5728.

The current behavior for splitting charts in the tile map visualization is broken. When there are multiple tile maps in one visualization, a user can drag the tiles of one map over another. This is undesired behavior and until a solution can be found to prevent this, the option for splitting charts should be removed. The gif below shows this broken behavior in action.

tilemap

@spalger spalger assigned stormpython and unassigned spalger Feb 1, 2016
@spalger
Copy link
Contributor

spalger commented Feb 1, 2016

tests are failing

@epixa
Copy link
Contributor

epixa commented Feb 8, 2016

This also needs to be rebased on master or have master merged into it.

@stormpython stormpython removed their assignment Mar 2, 2016
@stormpython
Copy link
Contributor Author

jenkins, test it.

@stormpython
Copy link
Contributor Author

This pr is failing because of a bug in grunt:esvm. See here.

@epixa
Copy link
Contributor

epixa commented Mar 3, 2016

jenkins, test it

@stormpython
Copy link
Contributor Author

jenkins, test it

@rashidkpc
Copy link
Contributor

That is to say, you won't be removing any code in this pull, you'll just be adding a new parameter to the schema, for example here: https://github.com/elastic/kibana/blob/master/src/plugins/kbn_vislib_vis_types/public/tile_map.js#L123

You might add to the Split Chart objects a deprecate: true key. Then in the agg editor you wouldn't show the "Split Chart" option when adding a bucket. You also would show a deprecation message in the editor for the split chart.

@epixa
Copy link
Contributor

epixa commented Mar 25, 2016

Perhaps we should also start showing a deprecation warning when people are using existing split charts? We could add a hideSplitChartWarning config value or something to let people disable the warning since it would be user facing, but this way they'd see the warning at least once and would know that it could potentially be removed in future versions.

@rashidkpc
Copy link
Contributor

@epixa jinx 😄

@stormpython
Copy link
Contributor Author

jenkins, test it

@stormpython
Copy link
Contributor Author

@rashidkpc ok, can you check this out now.

<div ng-if="agg.schema.deprecate" class="form-group">
<p class="vis-editor-agg-error">
"{{ agg.schema.title }}" has been deprecated.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is makes it sound like splitting of all charts has been deprecated. Maybe it would make sense to be able to provide a deprecation notice in src/plugins/kbn_vislib_vis_types/public/tile_map.js

@rashidkpc
Copy link
Contributor

Aside from being able to set a custom deprecation notice, this looks good.

@rashidkpc rashidkpc assigned stormpython and unassigned rashidkpc Mar 28, 2016
@stormpython
Copy link
Contributor Author

@rashidkpc I added a custom deprecation message option. Passing it back to you.

@stormpython stormpython assigned rashidkpc and unassigned stormpython Mar 29, 2016
@rashidkpc
Copy link
Contributor

LGTM

@rashidkpc rashidkpc merged commit 0597513 into elastic:master Mar 29, 2016
@epixa epixa changed the title Remove split chart option for tile map visualization Deprecate split chart option for tile map visualization Oct 26, 2016
@elasticmachine
Copy link
Contributor

Can one of the admins verify this patch?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants