Skip to content

Conversation

Suresh918
Copy link
Contributor

Fixes #15169

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Feb 13, 2019
Copy link
Contributor

@andrewseguin andrewseguin left a comment

Choose a reason for hiding this comment

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

Thanks for sending this PR @Suresh918.

Looks good to me, just a small nit. Can you change the jsDoc for active so that it does not say it is just the most recently sorted sortable?

Also, I think we ought to initialize it as an empty string now, so there's no ambiguity between undefined and ''.

  /** The id of the sorted MatSortable. */
  @Input('matSortActive') active: string = '';

@Suresh918
Copy link
Contributor Author

@andrewseguin Updated the JS doc. Can you please check.
Are there any ways to chat in private to discuss doubts ?

@andrewseguin
Copy link
Contributor

Cool thanks

Feel free to chat over the PR, it's nice for us to keep things documented for future reference. Would be happy to hear your thoughts/doubts on this

@andrewseguin andrewseguin added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Feb 19, 2019
@josephperrott josephperrott added the target: patch This PR is targeted for the next patch release label Mar 8, 2019
@mmalerba mmalerba added aaa and removed aaa labels Apr 25, 2019
@andrewseguin andrewseguin added the P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent label May 30, 2019
@mmalerba mmalerba removed the lgtm label Jul 31, 2020
@Suresh918
Copy link
Contributor Author

@andrewseguin When this pull request will be merged?

@andrewseguin
Copy link
Contributor

andrewseguin commented Feb 22, 2021

Unfortunately it takes a good amount of time to get changes in due to the myriad of tests that need to pass internally. I'll move this up to P2 to help prioritize it.

Do you mind rebasing so we'll be able to re-run tests when we're able

@andrewseguin andrewseguin added P2 The issue is important to a large percentage of users, with a workaround and removed P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent labels Feb 22, 2021
@Suresh918 Suresh918 requested a review from a team as a code owner February 25, 2021 22:10
…15169

sort: test case changes

test cases for mat sort active state for empty sequence and other sequences

lint errors fix

JS doc changes for sort active field
@Suresh918
Copy link
Contributor Author

Suresh918 commented Feb 25, 2021

@andrewseguin I created a new pull request for this with the same changes
#22029
as it is difficult to rebase as there are changes in the folders from the instant this merge request was created.

@annieyw
Copy link
Contributor

annieyw commented Mar 9, 2021

Closing as there is a newer version of this change. (#22029)

@annieyw annieyw closed this Mar 9, 2021
@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 Apr 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement P2 The issue is important to a large percentage of users, with a workaround target: patch This PR is targeted for the next patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Sort] If there is no direction, active should not be populated with a sortable ID

7 participants