-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Settings UI] Clamp vintage cursor height and history size #9370
Conversation
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.
@ the rest of the team - I think we should stick with 1-100 here, despite 25-100 being the valid values currently, because we should accept 1-100 as reasonable values. See #9175
@zadjii-msft I found out that the history size number box also has no bottom limit and allows the user to set negative values. Should we fix that? I could do it in this PR or file an issue. |
Yep we definitely should! I wouldn't mind if you included that in this PR. If this merges before you add that though, I'll make sure to file a follow up. Good catch! The min for that one should just be 0. |
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.
We should consider representing "Cursor Size" a slider if we're introducing a minimum/maximum. I'll file a follow-up task.
Thanks for doing this!
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 (
|
Change the vintage cursor height number box to a slider. ## References Related: #9370 ## PR Checklist * [x] Closes #9377 * [x] zadjii-msft edit: Now _this one_ closes #9175 * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [x] Schema updated. * [ ] ## Detailed Description of the Pull Request / Additional comments It seems like the cursor height couldn't be lower than 25 percent regardless of the given value, so I've changed the `MinCursorHeightPercent` in CustomTextRenderer header file. ## Validation Steps Performed Manual validation ![CursorHeightSlider](https://user-images.githubusercontent.com/39456018/110041939-bf076080-7d66-11eb-8d58-ba9a84922803.gif)
## Summary of the Pull Request Add `Minimum` and `Maximum` for the cursor height numberbox in the SUI. Add `Minimum` for the history size numberbox in the SUI. ## PR Checklist * [x] Closes #9357, Closes #9175 * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA ## Validation Steps Performed Manual validation (cherry picked from commit ac3fecb)
🎉 Handy links: |
🎉 Handy links: |
Summary of the Pull Request
Add
Minimum
andMaximum
for the cursor height numberbox in the SUI.Add
Minimum
for the history size numberbox in the SUI.PR Checklist
Validation Steps Performed
Manual validation