Skip to content

Conversation

@dtaylor113
Copy link
Member

@dtaylor113 dtaylor113 commented Jul 5, 2017

In the screenshot below:

  • Icons added to the Status column
  • Links added to the Name column

image

  • Truncated Text added to the Address column
    cell_tooltips

@akieling , @serenamarie125

JIRA: https://patternfly.atlassian.net/browse/OSUX-28

@cdcabrera cdcabrera self-requested a review July 10, 2017 13:47
@dlabrecq
Copy link
Member

dlabrecq commented Jul 10, 2017

So datatables.net allows users to provide the HTML table layout if they choose. In fact, we have an example of this in the patternfly test pages. Does the Angular implementation prevent that?

@dtaylor113
Copy link
Member Author

Hi @dlabrecq, Do you mean this: https://github.com/patternfly/patternfly/blob/master/tests/pages/_includes/widgets/table-view/js/config.js

Unfortunately, the angular-datatables plug-in cannot use the dt-column directive: "When using the angular way, you CANNOT use the dt-column directive. ...If you need to provide some options to your column, your must provide the dt-column-defs"

The dt-column-defs API does have a renderWith(r) call, but examples do not show embedded HTML. Also, the datatables HTML is embedded in the template of the a-pf pfTableView component and we don't expose it to the end users/devs.

But you raise a good point, let me try out the renderWith(r) call.

@dlabrecq
Copy link
Member

dlabrecq commented Jul 10, 2017

Nope, I'm speaking about HTML (DOM) sourced data as shown below.

https://github.com/patternfly/patternfly/blob/master/tests/pages/_includes/widgets/table-view/tmpl/table-all.html

The datatables.net allows a few ways of providing data:

I'm asking because that would eliminate the need for templates. If we don't prevent users from providing data in other ways, they could hook to a database, for example.

@dtaylor113
Copy link
Member Author

Hi, in angular-patternfly, we do not expose the underlying HTML to the end-user, it is in the component's HTML template. This allows an end-user to simply provide an array of item objects (the data) and an array of columns (defines column headers, etc..). While this allows for quick/easy setup, only one angular element/component, instead of a large amount of HTML. The trade-off is how does one customize a column/cell? This PR attempts to provide this.

I did try the renderWith API call:

ctrl.dtColumnDefs[3].renderWith (function (data, type, full) {
      return '<i>' + full.address + '</i>';
});

But unfortunately, the 'full' attribute is returning the HTML for the column, which includes a lot of commented out angular ng-if statements, not something like 'full.address" to reflect the underlying data, so I think HTML templates is the best way to go.
Thanks,

  • Dave

Copy link
Member

@dlabrecq dlabrecq left a comment

Choose a reason for hiding this comment

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

OK, just wanted to understand how the component works. LGTM

Copy link
Member

@cdcabrera cdcabrera left a comment

Choose a reason for hiding this comment

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

Minor copy updates. And a question about the jump back and forth between using LoDash and Angular $filter('filter'), is that a preference thing, or something I overlooked?

* @param {object} dtOptions Optional angular-datatables DTOptionsBuilder configuration object. See {@link http://l-lin.github.io/angular-datatables/archives/#/api angular-datatables: DTOptionsBuilder}
* @param {array} items Array of items to display in the table view.
* @param {array} columns Array of table column information to display in the table's header row
* @param {array} columns Array of table column information to display in the table's header row and optionaly render the cells of a column.
Copy link
Member

Choose a reason for hiding this comment

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

Minor... optionally

* @param {object} dtOptions Optional angular-datatables DTOptionsBuilder configuration object. See {@link http://l-lin.github.io/angular-datatables/archives/#/api angular-datatables: DTOptionsBuilder}
* @param {array} items Array of items to display in the table view.
* @param {array} columns Array of table column information to display in the table's header row
* @param {array} columns Array of table column information to display in the table's header row and optionaly render the cells of a 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 as above, optionally

Copy link
Contributor

Choose a reason for hiding this comment

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

@cdcabrera I think $filter('filter') is being used incorrectly in some cases where we need an exact match. I submitted a PR a while ago using lodash but it could have been pure javascript as well.

Copy link
Member

Choose a reason for hiding this comment

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

good good @akieling I'll add another inline callout on this PR


ctrl.getHTMLTemplate = function (key) {
var retVal = '';
var tableCol = $filter('filter')(ctrl.columns, {itemField: key});
Copy link
Member

Choose a reason for hiding this comment

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

Preference of LoDash vs $filter ... looks like acceptable behavior based on the documentation, personal preference, or something Angular specific?

Copy link
Member Author

Choose a reason for hiding this comment

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

Previously, we tried to limit our use of LoDash just to minimize upgrade concerns.

};

ctrl.handleColAction = function (key, value) {
var tableCol = $filter('filter')(ctrl.columns, {itemField: key});
Copy link
Member

Choose a reason for hiding this comment

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

Same LoDash vs $filter comment...

Copy link
Member Author

Choose a reason for hiding this comment

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

I think since this is an angular repo., it's better to use the angular $filter

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.

4 participants