-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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
Update default settings.json experience #5217
Conversation
I added a bunch of links to https://aka.ms/terminal-documentation. I think we agreed that it would be better to have links to more relevant spots but those need to be created. @cinnamon-msft could you create those aka.ms links so that I add them in? Or just let me know what to do here. @DHowett-MSFT Should we be using FW links here instead? I think we said no, but I want to make double-check. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we want aka.ms links here, but yea we should probably get more of them
@msftbot make sure @cinnamon-msft signs off on this |
Hello @zadjii-msft! Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:
If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you". |
Given the state of our documentation, I can't give specific links. I think just the one aka.ms documentation link is fine. |
I don’t think there is value in having a link above every section if every link is just the same link. It’s like saying “look here for specific documentation. NATCH! Got you, here’s the landing page.” |
I didn't realize we didn't have |
Hey, should we rename |
This is probably fine, and would probably still be fine with #3327. I'm mildly worried that renaming the setting now means we're breaking everyone who's got it set currently, unless they manually change it, because we won't be supporting migrating from the old value. We're also breaking other things too so idk |
I'm okay with changing it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NAK on the indentattion change unless you can explain what it is and why it is and where it came from, or fix the code to make it do 2 spaces by default.
[ | ||
// Copy and paste are bound to Ctrl+Shift+C and Ctrl+Shift+V in your defaults.json | ||
// These bindings additionally bind them to Ctrl+C and Ctrl+V | ||
// To learn more about selection, visit https://aka.ms/terminal-selection |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't necessarily think selection warrants its own URL callout
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was imagining the page would include the following:
- using the mouse and non-rebindable modifiers
- HTML Copy
- SingleLine
- how selection works with VTMM
Since HTML Copy is being discussed in #5212, I'm wondering if this will help with the discoverability there?
But we are just in the keybindings section so some of that isn't all that relevant. Whatcha think? Is this a valid concern?
Co-Authored-By: Dustin L. Howett (MSFT) <duhowett@microsoft.com>
This pull request introduces unexpanded variables (`%DEFAULT_PROFILE%`, `%VERSION%` and `%PRODUCT%`) to the user settings template and code to expand them. While doing this, I ran into a couple things that needed to widen from accepting strings to accepting string views. I also had to move application name and version detection up to AppLogic and expose the AppLogic singleton. The dynamic profile generation logic had to be moved to before we inject the templated variables, as the new default profile depends on the generated dynamic profiles. References #5189, #5217 (because it has a dependency on `VERSION` and `PRODUCT`). ## PR Checklist * [x] Closes #2721 * [x] CLA signed * [x] Tests added/passed * [ ] Requires documentation to be updated * [x] I've discussed this with core contributors already ## Validation Steps Performed Deleted my settings and watched them regenerate.
Co-Authored-By: Dustin L. Howett (MSFT) <duhowett@microsoft.com>
…microsoft/Terminal into dev/cazamor/settings-copy-pasta
Hello @carlos-zamora! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
🎉 Handy links: |
Summary of the Pull Request
Add comments and settings to settings.json for discoverability.
PR Checklist