feat(data-table): Add Column Sorting #261
Conversation
|
Changes Unknown when pulling 33c1c32 on feature/data-table-column-sort into * on master*. |
| @@ -0,0 +1,18 @@ | |||
| /* | |||
There was a problem hiding this comment.
should file be named column-sort.d.ts ?
There was a problem hiding this comment.
It looks like it could be but I don't think it should be. We don't have .d.ts files anywhere else except at the root.
There was a problem hiding this comment.
IMO, it should be a part of service file. If you separate type and interface in several files, it will never ending.
There was a problem hiding this comment.
Like a java project where each interface is in its own file? I feel like I saw a pattern somewhere with type definitions in their own files, but I can't recall where now. I think it's better practice to separate things out, but it's also more consistent with existing code-base to keep them together.
|
A few style things from trying the demo:
|
Didn't see that in the spec, but it sounds right now that you say it. I'll add a rule for that.
I noticed that, but I wasn't sure how to avoid it without having to style the column widths... it's really the table re-flowing to make the column wider based on the content width changing, not the labels shifting per se. Open to suggestions. |
Actually I can just add an empty block there so title size stays consistent. nm |
ollwenjones
force-pushed
the
feature/data-table-column-sort
branch
from
33c1c32
to
cbc74b2
Compare
|
Changes Unknown when pulling cbc74b2 on feature/data-table-column-sort into * on master*. |
|
I copy pasted some code and didn't use the cli for it, so I just realized it doesn't have that sanity check unit test... let me know if I need to add it in, but it sounded like they aren't even being run? |
ollwenjones
force-pushed
the
feature/data-table-column-sort
branch
from
cbc74b2
to
0acc3ed
Compare
|
Changes Unknown when pulling 0acc3ed on feature/data-table-column-sort into * on master*. |
| private colIdMap | ||
|
|
||
| constructor(private sortingService: MdDataColumnSortingService) { | ||
| this.colSortChanges$ = this.sortingService.sortingColumn$.subscribe((col) => { |
There was a problem hiding this comment.
the $ suffix doesn't seems to be in angular styleguide
There was a problem hiding this comment.
$ suffix is kind of standard indicator that a value is an Observable.
There was a problem hiding this comment.
Agree but not for Angular styleguide
There was a problem hiding this comment.
doesn't seem to be in
is precisely true. It does not seem to have an opinion about it. 😉
|
Thanks @justindujardin and @Gregcop1 for making time to review this. 👍 |
ollwenjones
force-pushed
the
feature/data-table-column-sort
branch
3 times, most recently
from
a374213
to
a7f5f81
Compare
|
Changes Unknown when pulling a7f5f81 on feature/data-table-column-sort into * on master*. |
|
Changes Unknown when pulling a7f5f81 on feature/data-table-column-sort into * on master*. |
|
Changes Unknown when pulling a7f5f81 on feature/data-table-column-sort into * on master*. |
| * @param sorting column & direction to sort by. | ||
| */ | ||
| public setSorting(sorting: ColumnSortingModel) { | ||
| this.sortingColumn$.next(sorting); |
There was a problem hiding this comment.
It's conventional way indicate a var is an observable... but I'll probably have to take it out, because it's not really a convention followed in the code-base so far.
|
@Gregcop1 @justindujardin What do we say about this, merge ready? Get rid of the |
|
IMO, we have to get rid of |
|
@Gregcop1 I'll take out the |
|
Reading test can be a kind of documentation so we have to be consistent. If you think current test format is too complex you must resolve this issue in another pull request (1 PR = 1 feature) where you revamp all the tests in one shot |
|
Ok. I'll try to find time this week. |
|
I was just thinking if "normal" test style has changed, we could gradually |
|
IMO, I prefer changing everything in one PR, cause gradually convert will never be finished ;) |
ollwenjones
force-pushed
the
feature/data-table-column-sort
branch
from
a7f5f81
to
ad4b7d5
Compare
|
@Gregcop1 I removed the |
ollwenjones
force-pushed
the
feature/data-table-column-sort
branch
from
ad4b7d5
to
84a5f82
Compare
|
Afraid I'm not real savvy on Travis. When the tests actually run, they are all passing, but it looks like IE9 is not connecting on SauceLabs. Anything I need to do here? |
|
@ollwenjones can you update this to the latest master? I removed IE9 and IE10, and merged the RC4 branch. Hopefully this problem will be resolved simply by updating. |
ollwenjones
force-pushed
the
feature/data-table-column-sort
branch
from
84a5f82
to
fa897b2
Compare
|
@justindujardin sorry I didn't see that right away. I just rebased on master. |
|
Woops, looks like I missed a some testing updates, sorry. |
Add column sorting via an attribute direcive which styles its host,
subsribes to and calls method on a service with an observable model.
Closes #144
ollwenjones
force-pushed
the
feature/data-table-column-sort
branch
from
fa897b2
to
c09c878
Compare
|
Hm... now I don't know @justindujardin. I had a bad import, but now my tests are passing, but a whole lot of others are failing. 😦 |
Add column sorting via an attribute directive which styles its host,
subsribes to and calls method on a service with an observable model.
Closes #144
I struggled initially in terms of where the sorting model / event listener should be housed, but decided on a service (rather than row or table component) since it allowed simplest implementation, and greatest flexibility for the user in terms of how/where they would like to listen to the column sorting changes.