Skip to content

Conversation

@abkieling
Copy link
Contributor

Add new configuration property to columns object to define a template function

The goal of this PR is to provide a fix to performance issues caused by the use of ng-include inside ng-repeat to parse and include the content of each table cell that has a template.
This change is backward compatible and doesn't affect existing code that depends on the htmlTemplate property of the columns object. This change adds a new property called tempateFn as a potential replacement to htmlTemplate.

Note: This new property is light and simple but doesn't support AngularJS directives in the template, therefore it doesn't support things like ng-click.

List of changes:

  • Add new templateFn property to documentation
  • Inject $sce into controller to tell AngularJS to trust the template HTML
  • Remove hasHTMLTemplate and getHTMLTemplate functions from controller because we can check whether the columns have a template by just checking the existence of the htmlTemplate and templateFn properties
  • Use ng-bind-html to invoke the template function and inject the results in the cells
  • Add a new unit test for this feature

PR Checklist

  • [ X ] Unit tests are included
  • Screenshots are attached (if there are visual changes in the UI)
  • A Designer (@beanh66) is assigned as a reviewer (if there are visual changes in the UI)
  • A CSS rep (@cshinn) is assigned as a reviewer (if there are visual changes in the UI)

* <ul style='list-style-type: none'>
* <li>.header - (string) Text label for a column header
* <li>.itemField - (string) Item field to associate with a particular column.
* <li>.templateFn - (function) (optional) Template function used to render each cell of the column. Example: <pre>templateFn: value => `<span class="text-danger">${value}</span>`</pre>
Copy link
Member

Choose a reason for hiding this comment

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

Please add some info. on the pros and cons of using templateFn as opposed to htmlTemplate. Pro: Is more performant than htmlTemplate. Con: Doesn't support AngularJS directives in the template, therefore it doesn't support things like ng-click.

* <li>.header - (string) Text label for a column header
* <li>.itemField - (string) Item field to associate with a particular column.
* <li>.templateFn - (function) (optional) Template function used to render each cell of the column. Example: <pre>templateFn: value => `<span class="text-danger">${value}</span>`</pre>
* <li>.htmlTemplate - (string) (optional) id/name of an embedded ng/html template. Ex: htmlTemplate="name_template.html". The template will be used to render each cell of the column.
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as above. Please include some info as to why an app dev might choose htmlTemplate over templateFn, or visa-versa. -thanks

Copy link
Member

@dtaylor113 dtaylor113 left a comment

Choose a reason for hiding this comment

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

LGTM! Nice solution!

Copy link
Member

@dtaylor113 dtaylor113 left a comment

Choose a reason for hiding this comment

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

Can we add an example of how one sets up and uses templateFn? Probably just adding the example you used in the unit tests to the ngDoc desc would be enough. -Thanks

@dtaylor113
Copy link
Member

Thanks @akieling , LGTM 👍

Copy link
Member

@dtaylor113 dtaylor113 left a comment

Choose a reason for hiding this comment

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

Hi @akieling, we have started using automatic semantic version releasing, so you will need to change your commit messages to a specific format, ex: "fix(pfTableView): ng-include inside ng-repeat hurts performance"

@dtaylor113 dtaylor113 merged commit dd0b784 into patternfly:master Aug 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants