Skip to content

Conversation

@jpuri
Copy link
Contributor

@jpuri jpuri commented Aug 8, 2014

I have added a boolean editor (checkbox). The changes are not final, but I want to have my changes reviewed so that I am sure that I have taken right approach.

I have assumed there will be a parameter "type" in "colDef".

@swalters
Copy link
Contributor

swalters commented Aug 8, 2014

@jpuri Thanks for the PR.
Instead of changing cellTextEditor.html to function as a checkbox, create a new template named cellBoolEditor.html and assign that template for booleans. That way, others can assign that template in editableCellTemplate and it gives a good pattern for future editors.

@jpuri
Copy link
Contributor Author

jpuri commented Aug 9, 2014

I have added separate template for boolean editor. I couple of points to mention:

  1. Following line (Line:131) in gridEdit.js can be removed, I just left it as removing it was creating trouble in doc generation:
    gridOptions.editableCellTemplate = gridOptions.editableCellTemplate || '';
  2. Since angular directve for checkbox was taking care of special handling(space keys, etc) needed for checkbox, I have reused 'uiGridTextEditor' directive (for focus, ESC key handling, etc).
    But yes having separate directive will also be good idea, as in future we might wish to add more functionality to it.
  3. I still need to add styling for checkbox positioning. Currently its more aligned to left and top, I think I would look better in center or at least less left than what it is currently.

Please review and share feedback.

@swalters
Copy link
Contributor

swalters commented Aug 9, 2014

Looks good. A few notes:

  1. In the edit tutorial, please add IsActive field to demonstrate boolean editing. http://localhost:9003/docs/#/tutorial/6_editable
  2. No idea why removing that line would interfere with docs gen. Maybe you need to move the doc above it to where you are defaulting the editableCellTemplate if it is undefined.
  3. This line not needed:
    colDef.type = colDef.type || 'text';
    type should be validated elsewhere and it would be string, never text
  4. case 'text': should be case 'string'
  5. If you look at ng-grid 2.x branch, you can probably find some nice css for the checkbox. Create a less directory in the edit feature and include that css there. see features\select\less for an example.
  6. I see a lot of style changes in the code so it makes it look like you changed a lot more than you really did. Do you have your editor set for 2 space indentions and convert tabs to spaces?
@jpuri
Copy link
Contributor Author

jpuri commented Aug 10, 2014

Hi Shane,

Thanks a lot for your feedback. I have incorporated all the changes you mentioned above.

In tutorial/6_editable, I have added column 'isActive' in grid and I have also done few more modifications on the page to add details about the booleanEditor.
In colDef options in the tutorial, I have added 'type' but default value I have kept as null. In (3) above you suggested to 'validate type somewhere else' but I a not sure where should I add that piece, or you have already done that.

swalters added a commit that referenced this pull request Aug 11, 2014
@swalters
Copy link
Contributor

squashed commits and merged

@swalters swalters closed this Aug 11, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants