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.

Sign up for GitHub

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

Jump to bottom

add a spec for initial position and launch mode #2654

Merged
merged 18 commits into from
Jan 7, 2020
Merged

add a spec for initial position and launch mode #2654

merged 18 commits into from
Jan 7, 2020

Conversation

KaiyuWang16
Copy link
Contributor

This PR is for the GitHub task: #1043

For now, we are going to implement two features:

  1. Users could set the initial position of the terminal window using the json settings file.
  2. Users could choose to maximize the window once the App is launched.

The current PR is only for the spec review. There is no code change yet.

References

PR Checklist

  • Closes #xxx
  • CLA signed. If not, go over here and sign the CLA
  • Tests added/passed
  • Requires documentation to be updated
  • I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

@KaiyuWang16 KaiyuWang16 changed the title Spec version 1 for set terminal initial position Spec for set terminal initial position Sep 3, 2019
@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Sep 6, 2019
@zadjii-msft zadjii-msft added Issue-Docs It's a documentation issue that really should be on MicrosoftDocs/Console-Docs Product-Terminal The new Windows Terminal. labels Sep 6, 2019
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Sep 6, 2019
@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Sep 11, 2019
@mdtauk
Copy link

mdtauk commented Sep 11, 2019

How do these initial position enum values work, with devices in Tablet Mode, or Hololens?

@carlos-zamora
Copy link
Member

The current PR is only for the spec review. There is no code change yet.

I see changes to the code though...

@KaiyuWang16
Copy link
Contributor Author

KaiyuWang16 commented Sep 16, 2019 •
edited
Loading

The current PR is only for the spec review. There is no code change yet.

I see changes to the code though...

Oh, sorry for this mistake. Those changes are just for ramp-up and research. I will create a new PR for code changes.

@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Sep 16, 2019
@zadjii-msft zadjii-msft mentioned this pull request Oct 1, 2019
Closed
@zadjii-msft
Copy link
Member

Hey it looks like this was never merged, but the feature was merged - @KaiyuWang16 can you come update this PR to reflect the state of the changes that were merged for this feature? I'd love to get it cleaned out of the PR backlog ☺️

@KaiyuWang16
Copy link
Contributor Author

Hey it looks like this was never merged, but the feature was merged - @KaiyuWang16 can you come update this PR to reflect the state of the changes that were merged for this feature? I'd love to get it cleaned out of the PR backlog ☺️

Updated ! Sorry I missed this.

zadjii-msft
zadjii-msft approved these changes Dec 19, 2019
View reviewed changes
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

I suppose the feature is merged so all I really have are formatting nits at this point. Thanks!

doc/specs/#1043 - Set the initial position of the Terminal/spec.md Outdated Show resolved Hide resolved
@zadjii-msft zadjii-msft added the Needs-Second It's a PR that needs another sign-off label Dec 19, 2019
@ghost ghost requested review from miniksa and carlos-zamora December 19, 2019 16:50
DHowett-MSFT
DHowett-MSFT suggested changes Dec 19, 2019
View reviewed changes
Copy link
Contributor

@DHowett-MSFT DHowett-MSFT left a comment

Choose a reason for hiding this comment

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

I do not understand why the PNG in this spec looks so strange.

image

How are you generating your screenshots?

Also, this looks like a screenshot that should just be a snippet of text...

@KaiyuWang16
Copy link
Contributor Author

I do not understand why the PNG in this spec looks so strange.

How are you generating your screenshots?
Also, this looks like a screenshot that should just be a snippet of text...

I updated the image and now it looks more clear.

@DHowett-MSFT
Copy link
Contributor

But does it need to be an image at all? It looks like the perfect candidate for a markdown fenced code block...

@KaiyuWang16
Copy link
Contributor Author

But does it need to be an image at all? It looks like the perfect candidate for a markdown fenced code block...

Changed. Thanks for the advice.

@DHowett-MSFT
Copy link
Contributor

@cinnamon-msft, @leonMSFT - we need one more (three) for a spec review. This one's old and the feature has already merged. Mind having a look?

@DHowett-MSFT DHowett-MSFT changed the title Spec for set terminal initial position add a spec for initial position and launch mode Jan 7, 2020
@DHowett-MSFT DHowett-MSFT merged commit 5119ed1 into master Jan 7, 2020
@DHowett-MSFT DHowett-MSFT deleted the dev/kawa/1043-set-initial-position-for-the-terminal branch January 7, 2020 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leonMSFT leonMSFT leonMSFT approved these changes

@zadjii-msft zadjii-msft zadjii-msft approved these changes

@DHowett-MSFT DHowett-MSFT DHowett-MSFT approved these changes

@bitcrazed bitcrazed bitcrazed left review comments

@miniksa miniksa Awaiting requested review from miniksa

@carlos-zamora carlos-zamora Awaiting requested review from carlos-zamora

Assignees
No one assigned
Labels
Issue-Docs It's a documentation issue that really should be on MicrosoftDocs/Console-Docs Needs-Second It's a PR that needs another sign-off Product-Terminal The new Windows Terminal.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@KaiyuWang16 @mdtauk @carlos-zamora @zadjii-msft @DHowett-MSFT @bitcrazed @leonMSFT