Skip to content

Hide and show columns, allow for zero to 3 columns, query param for numColumns#141

Merged
Gaurav0 merged 19 commits intoember-cli:masterfrom
Gaurav0:hide-and-show-cols
Aug 20, 2015
Merged

Hide and show columns, allow for zero to 3 columns, query param for numColumns#141
Gaurav0 merged 19 commits intoember-cli:masterfrom
Gaurav0:hide-and-show-cols

Conversation

@Gaurav0
Copy link
Copy Markdown
Contributor

@Gaurav0 Gaurav0 commented Aug 9, 2015

Relevant to #65, #100, #122

Comment thread app/gist/controller.js Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Might be nice to refactor this to use a computed property rather than an observer

@Gaurav0 Gaurav0 removed the working label Aug 11, 2015
@Gaurav0
Copy link
Copy Markdown
Contributor Author

Gaurav0 commented Aug 11, 2015

This PR is complete. Please review.

@Gaurav0 Gaurav0 mentioned this pull request Aug 14, 2015
@Gaurav0
Copy link
Copy Markdown
Contributor Author

Gaurav0 commented Aug 15, 2015

@stefanpenner @joostdevries @rwjblue Updated. Pretty please review.

Comment thread app/gist/controller.js
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm curious, it seems like rather then making this controller have array like qualities. Having the columns represented off-object would be clean. Something like:

controller.columns === [
  col1,
  col2,
  col3
]
columns: computed('dataColumnsNeed', function() {
  return ensureColums(this.get('dataColumnsNeed'))
});

the return columns object itself doesn't need to be an array, but may prove to be nice if it ways.

This would also make it easy to support N columns, if we are so inclined, but also begins to move to a pull based system over the imperative model.

some relevant code this would tidy up nicely:

@Gaurav0
Copy link
Copy Markdown
Contributor Author

Gaurav0 commented Aug 17, 2015

@stefanpenner Updated. Could you please review again?

Comment thread app/components/file-editor-column.js Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this CP at first glance appears to indicate, it will provide the last column. But after reading the implementation it seems to actually return true/false. Maybe it should be isLastColumn ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok, done.

Comment thread app/gist/controller.js Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should likely not put a complex object on the prototype, especially if it is meant to hold instance state.

Likley either set this in the constructor, or make it a cp.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Setting in constructor.

@Gaurav0
Copy link
Copy Markdown
Contributor Author

Gaurav0 commented Aug 18, 2015

@stefanpenner Are there any further issues or may I merge this?

Gaurav0 added a commit that referenced this pull request Aug 20, 2015
Hide and show columns, allow for zero to 3 columns, query param for numColumns
@Gaurav0 Gaurav0 merged commit 3957c7c into ember-cli:master Aug 20, 2015
@Gaurav0 Gaurav0 deleted the hide-and-show-cols branch March 21, 2016 13:49
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