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

Draggable rework #8442

Merged
merged 4 commits into from
Sep 23, 2016
Merged

Draggable rework #8442

merged 4 commits into from
Sep 23, 2016

Conversation

BigFunger
Copy link
Contributor

original PR: #6566

I attempted to consume the draggable-collection directives in the pipeline and ran into some weird behavior. This lead me to dig in a little deeper.

Changes

  • Modified agg_group.html to consume the group collection as the draggable-collection instead of vis.aggs. vis-aggs contains both the metrics and buckets aggregations. This caused the index manipulation to be erratic. (since the ui was based on a different, smaller array than the collection that was being manipulated)
  • Modified agg-group.js so that it re-orders the items in vis.aggs according to the modified order in group
  • Changed the test descriptors to use string templates instead of escaped characters per @epixa's suggestion
  • Removed the (reference) modifier to base.less to actually include the styles from dragula.css. Thanks @cjcenizal!!

@bevacqua
Copy link
Contributor

LGTM. These changes make it so that your new use case doesn't need any special handling, too, right?

@BigFunger
Copy link
Contributor Author

Correct. Not really sure how it was working correctly with the vis editor. :)

@bevacqua
Copy link
Contributor

bevacqua commented Sep 23, 2016 •
edited
Loading

It's a wonder how anything ever works

cjcenizal
cjcenizal approved these changes Sep 23, 2016
View reviewed changes
Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

Awesome! LGTM. 1 small suggestion. I did find a bug when trying to reorder the metrics buckets. The preview doesn't show up when you drag them. Is that within the scope of this PR?

src/core_plugins/kibana/public/visualize/editor/agg_group.js

//the aggs have been reordered in [group] and we need
//to apply that ordering to [vis.aggs]
const baseIndex = $scope.vis.aggs.indexOf($scope.group[0]);
Copy link
Contributor

Choose a reason for hiding this comment

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

It took me awhile to work out the role of this variable but once I did it made perfect sense. Totally going out on a limb here but do you think offsetIndex might be clearer/as clear? If not, then forget it. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed to indexOffset. Is that more clear?

@cjcenizal
Copy link
Contributor

You can ignore my comment about a problem with reordering metrics buckets. I just tried to repro and wasn't able to so it must have been user error the first time around.

@cjcenizal
Copy link
Contributor

Yup! Thanks!

On Friday, September 23, 2016, Jim Unger notifications@github.com wrote:

@BigFunger commented on this pull request.

In src/core_plugins/kibana/public/visualize/editor/agg_group.js
#8442:

@@ -43,7 +43,16 @@ uiModules
});

   $scope.$on('agg-drag-start', e => $scope.dragging = true);

  •  $scope.$on('agg-drag-end', e => $scope.dragging = false);

  •  $scope.$on('agg-drag-end', e => {

  •    $scope.dragging = false;

  •    //the aggs have been reordered in [group] and we need

  •    //to apply that ordering to [vis.aggs]

  •    const baseIndex = $scope.vis.aggs.indexOf($scope.group[0]);

renamed to indexOffset. Is that more clear?

—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
#8442, or mute the thread
https://github.com/notifications/unsubscribe-auth/ABLmg3AiakVAuEDZMmt2QJR1qWR5Z_esks5qtBYlgaJpZM4KEam9
.

@BigFunger BigFunger merged commit ebaed2f into elastic:master Sep 23, 2016
elastic-jasper added a commit that referenced this pull request Sep 23, 2016
@elastic-jasper
Backport PR #8442
e9c030e 
---------

**Commit 1:**
re-adds the dragula base styles

* Original sha: ee6962c
* Authored by Jim Unger <bigfunger@gmail.com> on 2016-09-22T21:57:29Z

**Commit 2:**
used string templates for test descriptors

* Original sha: 6797ca5
* Authored by Jim Unger <bigfunger@gmail.com> on 2016-09-22T22:00:01Z

**Commit 3:**
fixed some re-ordering edge cases and agg_group now uses group instead of vis.aggs

* Original sha: f4717fa
* Authored by Jim Unger <bigfunger@gmail.com> on 2016-09-22T22:01:18Z

**Commit 4:**
renamed baseIndex to indexOffset

* Original sha: 5d5b581
* Authored by Jim Unger <bigfunger@gmail.com> on 2016-09-23T18:09:38Z
elastic-jasper added a commit that referenced this pull request Sep 23, 2016
@elastic-jasper
Backport PR #8442
b233029 
---------

**Commit 1:**
re-adds the dragula base styles

* Original sha: ee6962c
* Authored by Jim Unger <bigfunger@gmail.com> on 2016-09-22T21:57:29Z

**Commit 2:**
used string templates for test descriptors

* Original sha: 6797ca5
* Authored by Jim Unger <bigfunger@gmail.com> on 2016-09-22T22:00:01Z

**Commit 3:**
fixed some re-ordering edge cases and agg_group now uses group instead of vis.aggs

* Original sha: f4717fa
* Authored by Jim Unger <bigfunger@gmail.com> on 2016-09-22T22:01:18Z

**Commit 4:**
renamed baseIndex to indexOffset

* Original sha: 5d5b581
* Authored by Jim Unger <bigfunger@gmail.com> on 2016-09-23T18:09:38Z
This was referenced Sep 23, 2016
Merged
Merged
BigFunger added a commit that referenced this pull request Sep 23, 2016
@BigFunger
Merge pull request #8467 from elastic/jasper/backport/8442/5.x
a4c3203 
[backport] PR #8442 to 5.x
BigFunger added a commit that referenced this pull request Sep 23, 2016
@BigFunger
Merge pull request #8466 from elastic/jasper/backport/8442/5.0
3da6cc3 
[backport] PR #8442 to 5.0
@epixa epixa added the v5.0.0 label Oct 26, 2016
airow pushed a commit to airow/kibana that referenced this pull request Feb 16, 2017
@elastic-jasper
Backport PR elastic#8442
2d28c72 
---------

**Commit 1:**
re-adds the dragula base styles

* Original sha: 5b0ed5c6609c3ac5cca014207d2b02873f6c9bcb [formerly ee6962c]
* Authored by Jim Unger <bigfunger@gmail.com> on 2016-09-22T21:57:29Z

**Commit 2:**
used string templates for test descriptors

* Original sha: 4bb21e6f606f15c45e580911e1df10e75ba19726 [formerly 6797ca5]
* Authored by Jim Unger <bigfunger@gmail.com> on 2016-09-22T22:00:01Z

**Commit 3:**
fixed some re-ordering edge cases and agg_group now uses group instead of vis.aggs

* Original sha: 2863994eca01cfc5dda347500fc5ccfbe52c2647 [formerly f4717fa]
* Authored by Jim Unger <bigfunger@gmail.com> on 2016-09-22T22:01:18Z

**Commit 4:**
renamed baseIndex to indexOffset

* Original sha: d21c636beca0e28e96c09954a6c47c36af53592e [formerly 5d5b581]
* Authored by Jim Unger <bigfunger@gmail.com> on 2016-09-23T18:09:38Z


Former-commit-id: e9c030e
airow pushed a commit to airow/kibana that referenced this pull request Feb 16, 2017
@BigFunger
Merge pull request elastic#8467 from elastic/jasper/backport/8442/5.x
2e7bd22 
[backport] PR elastic#8442 to 5.x

Former-commit-id: a4c3203
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjcenizal cjcenizal cjcenizal approved these changes

Assignees

@bevacqua bevacqua

@cjcenizal cjcenizal

Labels
review v5.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@BigFunger @bevacqua @cjcenizal @epixa