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

Fixes global chart title selection #6654

Merged
merged 10 commits into from
Apr 1, 2016
Merged

Fixes global chart title selection #6654

merged 10 commits into from
Apr 1, 2016

Conversation

stormpython
Copy link
Contributor

Closes #6556.

It fixes the issue where chart titles on the dashboard change based on the resizing of other charts on the dashboard. This is the result of a global selection of all x and y axis containing DOM elements.

This pull request fixes the issue by making the selection local and not global.

This pull request will need to be backported.

@rashidkpc
Copy link
Contributor

@stormpython these look like legit test failures.

@stormpython
Copy link
Contributor Author

So it seems. I will fix them.

@stormpython
Copy link
Contributor Author

@rashidkpc Tests are fixed.

jbudz
jbudz reviewed Mar 25, 2016
View reviewed changes
src/ui/public/vislib/lib/layout/splits/column_chart/chart_title_split.js
@@ -12,10 +14,10 @@ define(function () {
return function (selection) {
selection.each(function (data) {
var div = d3.select(this);
var parent = d3.select($(this).parents('.vis-wrapper')[0]);
Copy link
Member

Choose a reason for hiding this comment

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

does this need to be wrapped in d3.select? can we use only jquery, or d3 instead?

@jbudz
Copy link
Member

jbudz commented Mar 25, 2016

Small comment above, fix works for me.

Is there a better location we can manage which title to show ? Using a parent selector is giving me the feeling that this module is reaching outside its scope.

@jbudz jbudz assigned stormpython and unassigned jbudz Mar 25, 2016
@stormpython
Copy link
Contributor Author

@jbudz while I agree, this isn't a very pretty solution, just using jquery or only d3 looks much worse and more verbose. In addition, while I also agree reaching out of scope is generally a bad idea, due to the way the vislib currently works, to make that fix here would require a bigger change than what I've submitted. Given that the vislib will change in the future and these types of problems shouldn't happen with the new library, I'd rather not tinker with the fragile state of the current library.

@stormpython stormpython assigned jbudz and stormpython and unassigned stormpython and jbudz Mar 25, 2016
@stormpython
Copy link
Contributor Author

@jbudz ok, I found a less verbose way to do it with jquery. Making those changes.

@stormpython
Copy link
Contributor Author

ok @jbudz, back to you.

@stormpython stormpython assigned jbudz and unassigned stormpython Mar 25, 2016
@jbudz
Copy link
Member

jbudz commented Mar 25, 2016

LGTM

@jbudz jbudz assigned stormpython and unassigned jbudz Mar 25, 2016
@stormpython stormpython assigned spalger and unassigned stormpython Mar 28, 2016
@spalger
Copy link
Contributor

spalger commented Mar 29, 2016

LGTM, not seeing the issue rashid is (I don't think)

image

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

@rashidkpc @spalger and @jbudz both gave it a LGTM. Is it ok to merge in master?

@rashidkpc
Copy link
Contributor

@stormpython merge it

@rashidkpc
Copy link
Contributor

@stormpython also, please tag this with the versions it affects

@stormpython stormpython merged commit 52d3fbc into elastic:master Apr 1, 2016
@elastic-jasper elastic-jasper mentioned this pull request Apr 1, 2016
Closed
elastic-jasper added a commit that referenced this pull request Apr 1, 2016
@elastic-jasper
Backport PR #6654
e6bfb3e 
---------

**Commit 1:**
Troubleshooting

* Original sha: 81a9f29
* Authored by Shelby Sturgis <shelbyjsturgis@gmail.com> on 2016-03-22T21:44:17Z

**Commit 2:**
Merge branch 'master' into issue/6556

* Original sha: f628114
* Authored by Shelby Sturgis <shelbyjsturgis@gmail.com> on 2016-03-24T16:48:41Z

**Commit 3:**
troubleshooting

* Original sha: 152f2b6
* Authored by Shelby Sturgis <shelbyjsturgis@gmail.com> on 2016-03-24T17:13:01Z

**Commit 4:**
Closes #6556. Fixes issue with global x and y axis selection.

* Original sha: 2ffbb76
* Authored by Shelby Sturgis <shelbyjsturgis@gmail.com> on 2016-03-24T22:44:54Z

**Commit 5:**
Merge branch 'master' into issue/6556

* Original sha: da025aa
* Authored by Shelby Sturgis <shelbyjsturgis@gmail.com> on 2016-03-24T22:45:12Z

**Commit 6:**
Merge branch 'master' into issue/6556

* Original sha: cf4a152
* Authored by Shelby Sturgis <shelbyjsturgis@gmail.com> on 2016-03-24T22:45:50Z

**Commit 7:**
fixing tests

* Original sha: b2f26e7
* Authored by Shelby Sturgis <shelbyjsturgis@gmail.com> on 2016-03-25T16:55:30Z

**Commit 8:**
Merge branch 'master' into issue/6556

* Original sha: a56c5d5
* Authored by Shelby Sturgis <shelbyjsturgis@gmail.com> on 2016-03-25T17:51:21Z

**Commit 9:**
refactoring

* Original sha: b42f5bf
* Authored by Shelby Sturgis <shelbyjsturgis@gmail.com> on 2016-03-25T21:43:59Z

**Commit 10:**
Merge branch 'master' into issue/6556

* Original sha: 7f96a11
* Authored by Shelby Sturgis <shelbyjsturgis@gmail.com> on 2016-03-28T22:59:03Z
elastic-jasper added a commit that referenced this pull request Apr 1, 2016
@elastic-jasper
Backport PR #6654
fa99127 
---------

**Commit 1:**
Troubleshooting

* Original sha: 81a9f29
* Authored by Shelby Sturgis <shelbyjsturgis@gmail.com> on 2016-03-22T21:44:17Z

**Commit 2:**
Merge branch 'master' into issue/6556

* Original sha: f628114
* Authored by Shelby Sturgis <shelbyjsturgis@gmail.com> on 2016-03-24T16:48:41Z

**Commit 3:**
troubleshooting

* Original sha: 152f2b6
* Authored by Shelby Sturgis <shelbyjsturgis@gmail.com> on 2016-03-24T17:13:01Z

**Commit 4:**
Closes #6556. Fixes issue with global x and y axis selection.

* Original sha: 2ffbb76
* Authored by Shelby Sturgis <shelbyjsturgis@gmail.com> on 2016-03-24T22:44:54Z

**Commit 5:**
Merge branch 'master' into issue/6556

* Original sha: da025aa
* Authored by Shelby Sturgis <shelbyjsturgis@gmail.com> on 2016-03-24T22:45:12Z

**Commit 6:**
Merge branch 'master' into issue/6556

* Original sha: cf4a152
* Authored by Shelby Sturgis <shelbyjsturgis@gmail.com> on 2016-03-24T22:45:50Z

**Commit 7:**
fixing tests

* Original sha: b2f26e7
* Authored by Shelby Sturgis <shelbyjsturgis@gmail.com> on 2016-03-25T16:55:30Z

**Commit 8:**
Merge branch 'master' into issue/6556

* Original sha: a56c5d5
* Authored by Shelby Sturgis <shelbyjsturgis@gmail.com> on 2016-03-25T17:51:21Z

**Commit 9:**
refactoring

* Original sha: b42f5bf
* Authored by Shelby Sturgis <shelbyjsturgis@gmail.com> on 2016-03-25T21:43:59Z

**Commit 10:**
Merge branch 'master' into issue/6556

* Original sha: 7f96a11
* Authored by Shelby Sturgis <shelbyjsturgis@gmail.com> on 2016-03-28T22:59:03Z
@elastic-jasper elastic-jasper mentioned this pull request Apr 1, 2016
Closed
stormpython added a commit to stormpython/kibana that referenced this pull request Apr 4, 2016
@stormpython
Backporting pr elastic#6654 to 4.x branch
be9fd81
@stormpython stormpython mentioned this pull request Apr 4, 2016
Merged
@epixa epixa mentioned this pull request Apr 4, 2016
Merged
@stormpython stormpython added v4.5.0 and removed v4.5.1 labels Apr 4, 2016
@w33ble w33ble added v4.5.1 and removed v4.5.0 labels Apr 5, 2016
w33ble added a commit that referenced this pull request Apr 5, 2016
@w33ble
Merge pull request #6763 from stormpython/backport/6654
c60f047 
Backporting PR #6654 to 4.x
@epixa epixa added the v4.5.2 label May 14, 2016
@epixa epixa removed the v4.5.1 label May 14, 2016
@epixa epixa added v4.5.4 and removed v4.5.2 labels Jul 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
review v4.5.4 v4.6.0 v5.0.0-alpha1 v5.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Labels for split charts come and go depending on other split charts
6 participants
@stormpython @rashidkpc @jbudz @spalger @epixa @w33ble