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

Add support for multiple rulers with different colors #88453

Merged
merged 7 commits into from Feb 4, 2020

Conversation

@mallardduck
Copy link
Contributor

mallardduck commented Jan 10, 2020

This change set:

  • adds an interface for Ruler Color Options,
  • adds a union type of that interface and number,
  • adjust the rulers type hint to use said union type,
  • adds a mechanism to apply boxShadow to fastDomNode, and
  • updates the way rulers are rendered to support individual colors.

I've also ensured that descriptions are provided for settings.json. So now, all nodes will provide explanations.

There may be other ways to implement adjusting the boxShadow - certainly open for input if adding the setBoxShadow isn't as preferred.


This PR fixes #54312 by allowing ruler colors to be defined in settings.json.

This change set:
adds an interface for Ruler Color Options,
adds a union type of that interface and number,
adjust the rulers type hint to use said union type,
adds a mechanism to apply boxshadow to fastDomNode, and
updates the way rulers are rendered to support individual colors.

I've also ensured that descriptions are provided for settings.json.
So now, all nodes will provide explanations.
@msftclas

This comment has been minimized.

Copy link

msftclas commented Jan 10, 2020

CLA assistant check
All CLA requirements met.

mallardduck added 2 commits Jan 11, 2020
@methodbox

This comment has been minimized.

Copy link

methodbox commented Jan 12, 2020

@mallardduck
Looks like the first commit rejected. Take a look?

Thanks for doing the PR. I’m excited to use this feature!

@methodbox

This comment has been minimized.

Copy link

methodbox commented Jan 13, 2020

Looks like the merge failed due to a problem with the actual test, but not sure. Line 11 just says undefined.

Run .\scripts\test-integration.bat --tfs "Integration Tests"
Active code page: 65001
"Running integration tests out of sources."

undefined

  Search-integration

    √ Text: GameOfLife (51ms)

    √ Text: GameOfLife (RegExp) (57ms)

    √ Text: GameOfLife (unicode escape sequences)
@mallardduck

This comment has been minimized.

Copy link
Contributor Author

mallardduck commented Jan 13, 2020

@methodbox I think that's not the cause of the failure. If you check, the same test for different commits on this PR that message was being thrown in all of them without failure.

@mallardduck

This comment has been minimized.

Copy link
Contributor Author

mallardduck commented Jan 13, 2020

@methodbox, yeah looks fine now.

Just need someone from Microsoft to review, give feedback and/or accept it.

@mallardduck mallardduck requested a review from alexdima Jan 18, 2020
@mallardduck

This comment has been minimized.

Copy link
Contributor Author

mallardduck commented Jan 26, 2020

@connor4312 @alexdima - Hey there, I know that I read that multiple ruler colors was in a backlog for a later release point. However I'm wondering if we can get this merged in sooner than later, since I have provided a working implementation?

alexdima added 2 commits Feb 4, 2020
@alexdima alexdima added this to the February 2020 milestone Feb 4, 2020
@alexdima

This comment has been minimized.

Copy link
Member

alexdima commented Feb 4, 2020

Thank you!

@alexdima alexdima merged commit 42582a5 into microsoft:master Feb 4, 2020
4 of 5 checks passed
4 of 5 checks passed
linux
Details
windows
Details
darwin
Details
VS Code in progress
Details
license/cla All CLA requirements met.
Details
@methodbox

This comment has been minimized.

Copy link

methodbox commented Feb 6, 2020

@alexdima

Any idea when this will make it into distributed builds?

@alexdima

This comment has been minimized.

Copy link
Member

alexdima commented Feb 6, 2020

@methodbox This will make it to insiders in the next couple days (insiders is now frozen until stable is shipped), and will come to stable with the February stable release ~4weeks from nwo

@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.