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

fix(sort): display mat-sort-header arrow when sorting programmatically #13171

Closed
wants to merge 2 commits into from

Conversation

adgoncal
Copy link
Contributor

Fix mat-sort-header arrow not displaying after sorting programmatically (eg. matSort.sort())

Related to #10242, #12754

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Sep 17, 2018
@jelbourn
Copy link
Member

@andrewseguin can you review?

@andrewseguin
Copy link
Contributor

My apologies for the late review, we should have checked this out sooner. Thanks for making this pull request, everything looks great.

The only nit I see with this implementation is that there is no animation when sort is activated programatically. It looks like the arrow just appears. This also comes up when you cycle through a header's sort state (active -> asc -> desc -> inactive -> active). The second active does not show an animation, but just appears without floating in.

These are minor issues and I'd rather see this fix go in even if it means a small regression in animations.

If you get some time, could you take a look into see if you can resolve this small issue? If not, let's go ahead and get this merged.

Thanks again for the PR

@andrewseguin andrewseguin added pr: lgtm target: patch This PR is targeted for the next patch release labels Feb 7, 2019
@molayodecker
Copy link

Can you please fix circle so that this PR can be merged? @andrewseguin @leduyminh48

@adgoncal
Copy link
Contributor Author

@andrewseguin: I don't think I will have the time to look into the animations issue in the near future.

@mmalerba mmalerba added aaa and removed aaa labels Apr 25, 2019
@adgoncal adgoncal force-pushed the fix-sort-header-arrow branch from 25720a6 to 539ed14 Compare May 14, 2019 20:21
@swseverance
Copy link
Contributor

@andrewseguin @leduyminh48 any idea when this PR will be merged? It's been outstanding for awhile. Thank you

Fix mat-sort-header arrow not displaying after sorting programmatically (eg. `matSort.sort()`)

Related to angular#10242, angular#12754
@adgoncal adgoncal force-pushed the fix-sort-header-arrow branch from 539ed14 to 410bd64 Compare September 6, 2019 14:30
@adgoncal adgoncal force-pushed the fix-sort-header-arrow branch from 410bd64 to 0d02c5a Compare September 6, 2019 15:20
@adgoncal
Copy link
Contributor Author

adgoncal commented Sep 6, 2019

Unexpected value 'MatFormFieldModule in /b/f/w/bazel-out/k8-fastbuild/bin/src/material/form-field/form-field-module.d.ts' imported by the module 'TableExamplesModule in /b/f/w/src/material-examples/material/table/module.ts'. Please add a @NgModule annotation.

Unexpected value 'MatSelectModule in /b/f/w/bazel-out/k8-fastbuild/bin/src/material/select/select-module.d.ts' imported by the module 'TableExamplesModule in /b/f/w/src/material-examples/material/table/module.ts'. Please add a @NgModule annotation.

Why is it unexpected? It is just a regular module import. This error tells me nothing useful.

@jelbourn, @andrewseguin I don't have time to debug this. This PR has been open for almost a whole year now. If you have any intention of fixing this bug, please take over from here. Otherwise, feel free to close this PR.

@andrewseguin
Copy link
Contributor

This should be captured by #19492

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Jul 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes PR author has agreed to Google's Contributor License Agreement target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants