Adding check-box component #11550

ycombinator · 2017-05-01T18:56:51Z

This PR adds a component for a checkbox input. It can be used as follows:

<check-box
  id="itemId"
  is-selected="isItemSelected"
  on-select-change="functionToCallWhenSelectedStateChanges"
></check-box>

functionToCallWhenSelectedStateChanges will be called with two argument:

itemId, and
newIsSelected

BigFunger · 2017-05-01T20:20:31Z

This component still relies on the two-way ng-model binding to make sure both the component and the consumer has the valid checked state. I spent some time this afternoon trying to refactor it so that the consumer controlled the state change, but I had little success. Abandoning the effort for now.

Ideally, that's how it would be structured... something like this:

selectable_row.html

<input
  type="checkbox"
  class="kuiCheckBox"
  ng-click="selectableRow.onSelectClick()"
  ng-model="selectableRow.selected"
>

selectable_row.js

app.directive('selectableRow', function () {
  return {
    scope: {
      item: '=',
      selected: '=',
      onClick: '=',
    },
    controller: class SelectableRowController {
      onSelectClick = () => {
        this.onClick(this.item);
      }
    }
  };
});

consumer.html

<selectable-row
  item="item.watch"
  selected="item.selected"
  on-click="onSelectedClick"
></selectable-row>

consumer.js

$scope.onSelectedClick = (item) => {
  const myItem = findFromState(item);
  myItem.selected = !myItem.selected;
};

cjcenizal

LGTM! Code looks like it's still working.

cjcenizal · 2017-05-02T15:28:45Z

As we add more table-related components, we may want to consider grouping them in a table directory.

ycombinator · 2017-05-02T17:21:57Z

Chatting with @BigFunger on Zoom, here's another version to try out once we have a testbed:

selectable_row.html

<input
  type="checkbox"
  class="kuiCheckBox"
  ng-click="selectableRow.onSelectClick()"
  ng-model="selectableRow.selected"
>

selectable_row.js

app.directive('selectableRow', function () {
  return {
    scope: {
      id: '=',
      selected: '=',
      onClick: '=',
    },
    controller: class SelectableRowController {
      onSelectClick = () => {
        this.onClick(this.id);
      }
    }
  };
});

consumer.html

<selectable-row
  id="item.id"
  selected="item.selected"
  on-click="onSelectedClick"
></selectable-row>

consumer.js

$scope.items = [];

function findFromState(id) {
   return _.find($scope.items, item => item.id === id);
}

$scope.onSelectedClick = (id) => {
  const myItem = findFromState(id);
  myItem.selected = !myItem.selected;
};

ycombinator · 2017-05-08T14:44:33Z

This PR is waiting on #11571 to be merged. Once that is merged, I will modify this PR to use the testbed and demo the component in this PR.

ycombinator · 2017-05-09T00:33:42Z

@BigFunger @cjcenizal I've updated this PR to add in example usage of the selectable-row component on the new testbed. To see it in action, check out this PR and visit http://localhost:5601/app/kibana#/testbed.

After experimenting with this component's interface in several different ways on the testbed (yay!) I ended up changing it, particularly the semantics of the onSelectChange function. Angular's two-way binding already takes care of updating the selected/unselected state, so I decided that the intent of the onSelectChange function should be more about conveying the updated state to the parent/consumer component, should it want to do something with it (like setting a message in the UI as the testbed consumer is doing).

Let me know what you think of this change. Thanks!

cjcenizal

Looks great! I had just a couple nitpicky suggestions, but my main question is about the name. Can we just go with kui-check-box as the name of the directive?

cjcenizal · 2017-05-09T01:11:01Z

src/core_plugins/testbed/public/testbed.html

+              id="item.id"
+              is-selected="item.isSelected"
+              on-select-change="testbed.onSelectChange"
+            ></selectable-row>


Looks good! Thought tbh, from looking at the interface and implementation, wouldn't it make more sense to just call this directive check-box or kui-check-box? I think we'll end up implementing a React component that does the same thing, and we'll probably just call it KuiCheckBox because that's pretty much all it's concerned with. It could be used in a table row, a form, etc.

Yeah, I'm good with your proposal. It also eliminates the need for another component that I was about to promote into OSS Kibana: selectable-column, which is intended to be used for a "select all" functionality. It makes sense to me that there should be a generic check box component that could be used in various contexts.

So I'm +1 for calling this component check-box (looking through other Angular directives in Kibana, we don't have the kui prefix convention anywhere so I don't want to introduce it now).

@BigFunger Your thoughts before I make this broad change affecting multiple places in this PR?

I do not have a problem with it. Don't remember if there was a solid reason for making this two components initially, but the functionality of them does seem to have converged. Let me/us know if you find any weirdness as you implement the change.

cjcenizal · 2017-05-09T01:11:37Z

src/ui/public/selectable_row/selectable_row.js

+  return {
+    restrict: 'E',
+    replace: true,
+    transclude: true,


I don't think we need this property, do we?

cjcenizal · 2017-05-09T01:12:02Z

src/ui/public/selectable_row/selectable_row.js

+    restrict: 'E',
+    replace: true,
+    transclude: true,
+    template: template,


This could just be template, since the variable assigned as the value is the same name as the property.

cjcenizal · 2017-05-09T01:13:45Z

src/ui/public/selectable_row/selectable_row.js

+      isSelected: '=',
+      onSelectChange: '=',
+    },
+    controllerAs: 'selectableRow',


You may have already settled on a convention about this, but my 2c are that this may not be necessary. IMO the template is just as readable (if not slightly more so) like this:

<input type="checkbox" class="kuiCheckBox" ng-click="this.onSelectChange(this.id, this.isSelected)" ng-model="this.isSelected" >

I don't think we have an established convention around this in Kibana at the moment.

I think the main pattern we are trying to avoid here is hanging properties directly of $scope in the controller JS code (e.g. $scope.isSelected) and referring to them with their bare names (e.g. isSelected) in the corresponding template. As long as that principle is followed, I'm good with either using this or a named controller via controllerAs in the template.

@BigFunger Do you have a strong opinion either way on this?

I had received direction from @spalger a while ago to implement controllerAs, and we have consistently applied it across all of the Watcher UI. So all things being equal, I would like to continue down that path unless there is a strong reason not to.

I kind of like the way that this.isSelected mirrors the way it's accessed within the context of the controller, but this in templates normally references $scope, so we would have to use controllerAs: 'this' and that might cause more confusion (especially for people used to accessing $scope with this) than it's worth...

That said, naming is hard. My preference, in order, is:

controllerAs: 'selectableRow'

controllerAs: 'controller'

controllerAs: 'this'

(Moving @cjcenizal's comment into this thread to keep the discussion flow)

Ah I see. I agree with you Spencer. I think "controllerAs: this" is really
confusing. So I'd be happy with either 1 or 2, or not using controllerAs in
this case.

Thanks for the discussion, folks. I'm going with option 1, which means keeping the code in this PR as-is.

cjcenizal · 2017-05-09T17:17:41Z

Ah I see. I agree with you Spencer. I think "controllerAs: this" is really confusing. So I'd be happy with either 1 or 2, or not using controllerAs in this case.

…

On Tue, May 9, 2017 at 10:08 AM Spencer ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/ui/public/selectable_row/selectable_row.js <#11550 (comment)>: > +import template from './selectable_row.html'; + +const app = uiModules.get('kibana'); + +app.directive('selectableRow', function () { + return { + restrict: 'E', + replace: true, + transclude: true, + template: template, + scope: { + id: '=', + isSelected: '=', + onSelectChange: '=', + }, + controllerAs: 'selectableRow', I kind of like the way that this.isSelected mirrors the way it's accessed within the context of the controller, but this in templates normally references $scope, so we would have to use controllerAs: 'this' and that might cause more confusion (especially for people used to accessing $scope with this) than it's worth... That said, naming is hard. My preference, in order, is: 1. controllerAs: 'selectableRow' 2. controllerAs: 'controller' 3. controllerAs: 'this' — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#11550 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABLmg-V5yHnJMUgLC-pnU306_JxpY-1Hks5r4J2VgaJpZM4NNUGO> .

ycombinator · 2017-05-09T18:11:33Z

@cjcenizal @BigFunger I've updated this PR with the latest feedback. It's ready for re-review. Thanks!

cjcenizal

Looks great! Very readable.

BigFunger · 2017-05-10T15:11:34Z

I would have liked to see a 'all selected' example in this testbed since this one control is intended to replace two existing components. I created one myself (linked here just for reference):
testbed.js
testbed.html

My example worked, so:
LGTM!

ycombinator · 2017-05-10T16:11:58Z

Testbed reset via f724bf5. Just FYI, I was able to do this nicely by running git checkout master src/core_plugins/testbed/ before committing the changes.

* Adding selectable-row component * Adding example usage to testbed * Using shorthand property name * No need for transclusion * Renaming selectable-row to check-box * Resetting testbed code

ycombinator · 2017-05-10T16:13:51Z

Backported to:

5.x / 5.5: 93fd1c5

* Adding selectable-row component * Adding example usage to testbed * Using shorthand property name * No need for transclusion * Renaming selectable-row to check-box * Resetting testbed code

ycombinator requested review from cjcenizal and BigFunger May 1, 2017 18:57

ycombinator added the review label May 1, 2017

cjcenizal approved these changes May 2, 2017

View reviewed changes

ycombinator mentioned this pull request May 2, 2017

Testbed for aiding development in Kibana #11571

Merged

Adding selectable-row component

71af7d6

ycombinator force-pushed the promote-components-to-oss/selectable-row branch from b17ae74 to 71af7d6 Compare May 8, 2017 23:40

Adding example usage to testbed

733b536

cjcenizal suggested changes May 9, 2017

View reviewed changes

ycombinator added 2 commits May 9, 2017 02:48

Using shorthand property name

f4cefc8

No need for transclusion

9cbe587

Renaming selectable-row to check-box

12f5f78

ycombinator changed the title ~~Adding selectable-row component~~ Adding check-box component May 9, 2017

cjcenizal approved these changes May 9, 2017

View reviewed changes

BigFunger approved these changes May 10, 2017

View reviewed changes

Resetting testbed code

f724bf5

ycombinator merged commit e14ec93 into elastic:master May 10, 2017

ycombinator added v5.5.0 v6.0.0 labels May 10, 2017

ycombinator deleted the promote-components-to-oss/selectable-row branch May 12, 2017 23:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding check-box component #11550

Adding check-box component #11550

ycombinator commented May 1, 2017 •

edited

Loading

BigFunger commented May 1, 2017 •

edited

Loading

cjcenizal left a comment

cjcenizal commented May 2, 2017 •

edited

Loading

ycombinator commented May 2, 2017

ycombinator commented May 8, 2017

ycombinator commented May 9, 2017

cjcenizal left a comment

cjcenizal May 9, 2017

ycombinator May 9, 2017 •

edited

Loading

BigFunger May 9, 2017

cjcenizal May 9, 2017

cjcenizal May 9, 2017

cjcenizal May 9, 2017

ycombinator May 9, 2017

BigFunger May 9, 2017

spalger May 9, 2017

ycombinator May 9, 2017 •

edited

Loading

ycombinator May 9, 2017

cjcenizal commented May 9, 2017 via email

ycombinator commented May 9, 2017

cjcenizal left a comment

BigFunger commented May 10, 2017

ycombinator commented May 10, 2017

ycombinator commented May 10, 2017

Adding check-box component #11550

Adding check-box component #11550

Conversation

ycombinator commented May 1, 2017 • edited Loading

BigFunger commented May 1, 2017 • edited Loading

cjcenizal left a comment

Choose a reason for hiding this comment

cjcenizal commented May 2, 2017 • edited Loading

ycombinator commented May 2, 2017

ycombinator commented May 8, 2017

ycombinator commented May 9, 2017

cjcenizal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ycombinator May 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ycombinator May 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal commented May 9, 2017 via email

ycombinator commented May 9, 2017

cjcenizal left a comment

Choose a reason for hiding this comment

BigFunger commented May 10, 2017

ycombinator commented May 10, 2017

ycombinator commented May 10, 2017

ycombinator commented May 1, 2017 •

edited

Loading

BigFunger commented May 1, 2017 •

edited

Loading

cjcenizal commented May 2, 2017 •

edited

Loading

ycombinator May 9, 2017 •

edited

Loading

ycombinator May 9, 2017 •

edited

Loading