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 issues with keybindings list header behavior (#41558) #60217

Merged
merged 1 commit into from Jan 4, 2019

Conversation

Projects
None yet
5 participants
@reima
Copy link
Contributor

reima commented Oct 9, 2018

Resolves #41558

Fixes all the issues displayed in #41558 (comment) by moving the list header out from the list as suggested in #41789 (comment). This also makes the list header stay in place when scrolling the list, which makes it easier to make sense of the columns.

@msftclas

This comment has been minimized.

Copy link

msftclas commented Oct 9, 2018

CLA assistant check
All CLA requirements met.

@sandy081 sandy081 self-requested a review Oct 12, 2018

@sandy081 sandy081 added this to the October 2018 milestone Oct 12, 2018

@sandy081

This comment has been minimized.

Copy link
Member

sandy081 commented Oct 12, 2018

There is already a PR open for this #59202

@sandy081 sandy081 closed this Oct 12, 2018

@sandy081 sandy081 removed this from the October 2018 milestone Oct 12, 2018

@reima

This comment has been minimized.

Copy link
Contributor Author

reima commented Oct 12, 2018

@sandy081 Thank you for taking a look at this. PR #59202 only addresses part of the issue, and does not follow the solution you proposed in #41789 (comment). Would you please reconsider my PR? Thanks.

@sandy081

This comment has been minimized.

Copy link
Member

sandy081 commented Oct 15, 2018

@reima I see the other PR has the changes I requested.

@reima

This comment has been minimized.

Copy link
Contributor Author

reima commented Oct 16, 2018

@sandy081 Do you mean #41789 with "the other PR"? That one has the requested changes, but the author seems to have to intention of getting it updated and merged (they closed it about 5 months ago). PR #59202 on the other hand only fixes the focus problem, but does not move the list header out of the list.

So as I see it there is one complete, but outdated and closed PR (#41789), one incomplete PR (#59202), and one complete PR with an author that just wants to see this issue fixed (this one) 😄

I don't care much about which PR gets merged, as long as it fixes #41558. But I've taken the time to make this PR because there was no other complete and up to date one for this issue. So is there anything I can do to move this forward?

@sandy081

This comment has been minimized.

Copy link
Member

sandy081 commented Oct 19, 2018

@reima You are right. I am opening this PR for the complete fix.

@sandy081 sandy081 reopened this Oct 19, 2018

@sandy081 sandy081 added the extensions label Oct 19, 2018

@sandy081 sandy081 added this to the October 2018 milestone Oct 19, 2018

@reima reima force-pushed the reima:issue/41558 branch from 9ef1978 to 1c80fac Oct 20, 2018

@reima

This comment has been minimized.

Copy link
Contributor Author

reima commented Oct 20, 2018

I've rebased to master. Now one of the unit tests fails on Azure Pipelines. The test seems totally unrelated to the changes, and I cannot reproduce the failure locally. Is there a way to re-run the Azure Pipelines build to determine if the test is flaky?

@sandy081

This comment has been minimized.

Copy link
Member

sandy081 commented Oct 22, 2018

Thanks @reima. Will take a look.

@reima reima force-pushed the reima:issue/41558 branch from 1c80fac to b470a3d Dec 17, 2018

@sandy081 sandy081 merged commit 4fd5c09 into Microsoft:master Jan 4, 2019

1 of 2 checks passed

VS Code #20181217.1 failed
Details
license/cla All CLA requirements met.
Details
@sandy081

This comment has been minimized.

Copy link
Member

sandy081 commented Jan 4, 2019

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment