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.

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

Implement column grouping #24

Merged
merged 6 commits into from
Jul 13, 2017
Merged

Implement column grouping #24

merged 6 commits into from
Jul 13, 2017

Conversation

AdamWard1995
Copy link
Contributor

This project uses semver, please check the scope of this pr:

  • #none# - documentation fixes and/or test additions
  • #patch# - backwards-compatible bug fix
  • #minor# - adding functionality in a backwards-compatible manner
  • #major# - incompatible API change

This commit will enable grouping of columns into a "category"

CHANGELOG

  • Added functionality for grouping columns

@AdamWard1995
Copy link
Contributor Author

screen shot 2017-07-12 at 9 45 52 am

this.$(cellSelector).toArray().forEach((el) => {
width += $(el).outerWidth() + FUDGE_FACTOR
Copy link
Member

Choose a reason for hiding this comment

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

We don't have this problem anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not with flexbox approach to the fixed-table header


const width = this._calculateWidth(`${headerMiddleSelector} ${cellRowSelector} .frost-table-cell`)
const cssWidth = `${width}px`
this.$(`${headerMiddleSelector} .frost-table-header`).css({
Copy link
Member

Choose a reason for hiding this comment

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

Duplication of the object created from 331 to 334 and 335 to 338

@@ -412,6 +398,30 @@ export default Component.extend({
})
},

alignColumns (headerSelecter, bodySelector) {
const cellRowSelector = this.$(`${headerSelecter} .frost-table-header-columns`).length === 1
Copy link
Member

Choose a reason for hiding this comment

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

Duplication from 326

const width = bodyCellWidth > headerCellWidth ? bodyCellWidth : headerCellWidth

const cssWidth = `${width}px`
curHeaderCell.css({
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 (same object created twice)

columns.forEach((column) => {
if (column.category) {
hasCategories = true
return
Copy link
Member

Choose a reason for hiding this comment

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

This will not stop the loop.

let position = index + (selectable ? 2 : 1)
this.setCellWidths(position)
})
didRender () {
Copy link
Member

Choose a reason for hiding this comment

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

Why are you changing the hook?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

made more sense than scheduling something on the run queue

totalWidth += this.setCellWidths(1)
}
this.columns.forEach((column, index) => {
let position = index + (selectable ? 2 : 1)
Copy link
Member

Choose a reason for hiding this comment

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

const

totalWidth += this.setCellWidths(1)
}
this.columns.forEach((column, index) => {
let position = index + (selectable ? 2 : 1)
Copy link
Member

Choose a reason for hiding this comment

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

2 or 1 ? What is 2 and 1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as above, account for selection column

{{/if}}
{{#each _categoryColumns as |column|}}
{{frost-table-cell
cellRenderer=column.renderer
Copy link
Member

Choose a reason for hiding this comment

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

we are not using the headerRenderer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we are, check out the CP

@@ -13,6 +13,9 @@ import {PropTypes} from 'ember-prop-types'
* @property {String} propertyName - the name of the property in the data record to display in this column
* @property {Boolean} [frozen=false] - true if this column should be frozen (on either the left or right side of the table)
* @property {Component} [renderer] - the cell renderer to use for all data cells in this column
* @property {String} [category] - the category the column belongs to
* @property {String} [category] - the css class name of the category the column belongs to
Copy link
Member

Choose a reason for hiding this comment

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

category twice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch :)

@dafortin
Copy link
Member

Build failing because of linting errors.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling a6ec68f on AdamWard1995:columnGrouping into ** on ciena-frost:master**.

@AdamWard1995 AdamWard1995 merged commit e41b96f into ciena-frost:master Jul 13, 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.

3 participants