-
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
No longer load content dialogs when there is already one being shown #12517
Conversation
…/pabhoj/close_tabs_dialog
Hello @PankajBhojwani! 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 (
|
DefaultButton="Primary" /> | ||
|
||
<ContentDialog x:Name="ControlNoticeDialog" | ||
x:Uid="ControlNoticeDialog" | ||
x:Load="False" | ||
CloseButtonClick="_DialogCloseClick" |
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.
hey does this still work when the user presses Esc?
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.
Yep! We still end up in TerminalPage::_DialogCloseClick
upon hitting Esc
…12517) ## Summary of the Pull Request Somehow, the controls v2 update caused an issue where if you as much as _load_ a content dialog when there's already one open, we get holes in the terminal window (#12447) This commit introduces logic to `TerminalPage` to check whether there is a content dialog open before we try to load another one. ## PR Checklist * [x] Closes #12447 * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [ ] Tests added/passed * [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx * [ ] Schema updated. * [x] I work here ## Validation Steps Performed Can no longer repro #12447 (cherry picked from commit 788d33c)
…12517) Somehow, the controls v2 update caused an issue where if you as much as _load_ a content dialog when there's already one open, we get holes in the terminal window (#12447) This commit introduces logic to `TerminalPage` to check whether there is a content dialog open before we try to load another one. * [x] Closes #12447 * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [ ] Tests added/passed * [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx * [ ] Schema updated. * [x] I work here Can no longer repro #12447
🎉 Handy links: |
…#12840) After switching to ControlsV2, it seems that delay-loading a dialog causes the ContentDialog to be assigned a Height equal to it's content size. If we DON'T assign the ContentDialog a Row, I believe it's assigned Row 0 by default. So, when the dialog gets opened, the dialog seemingly causes a giant hole to appear in the body of the app. Assigning all the dialogs to Row 2 (where the rest of the content is) makes the "hole" appear in the same space as the rest of the TabContent, fixing the issue. Note that the actual content in a content dialog gets parented to the PopupRoot, so it actually always appeared in the correct place, it's just this weird hole that appeared in Row 0. * [x] Closes #12775 * See also: * #12202 was fixed by #12208 * #12447 was fixed by #12517 * [x] Tested manually * [x] Reverts #12625 * [x] Reverts #12517
…#12840) After switching to ControlsV2, it seems that delay-loading a dialog causes the ContentDialog to be assigned a Height equal to it's content size. If we DON'T assign the ContentDialog a Row, I believe it's assigned Row 0 by default. So, when the dialog gets opened, the dialog seemingly causes a giant hole to appear in the body of the app. Assigning all the dialogs to Row 2 (where the rest of the content is) makes the "hole" appear in the same space as the rest of the TabContent, fixing the issue. Note that the actual content in a content dialog gets parented to the PopupRoot, so it actually always appeared in the correct place, it's just this weird hole that appeared in Row 0. * [x] Closes #12775 * See also: * #12202 was fixed by #12208 * #12447 was fixed by #12517 * [x] Tested manually * [x] Reverts #12625 * [x] Reverts #12517 (cherry picked from commit c4e5ebf) Service-Card-Id: 80266902 Service-Version: 1.12
…#12840) After switching to ControlsV2, it seems that delay-loading a dialog causes the ContentDialog to be assigned a Height equal to it's content size. If we DON'T assign the ContentDialog a Row, I believe it's assigned Row 0 by default. So, when the dialog gets opened, the dialog seemingly causes a giant hole to appear in the body of the app. Assigning all the dialogs to Row 2 (where the rest of the content is) makes the "hole" appear in the same space as the rest of the TabContent, fixing the issue. Note that the actual content in a content dialog gets parented to the PopupRoot, so it actually always appeared in the correct place, it's just this weird hole that appeared in Row 0. * [x] Closes #12775 * See also: * #12202 was fixed by #12208 * #12447 was fixed by #12517 * [x] Tested manually * [x] Reverts #12625 * [x] Reverts #12517 (cherry picked from commit c4e5ebf) Service-Card-Id: 80266903 Service-Version: 1.13
Summary of the Pull Request
Somehow, the controls v2 update caused an issue where if you as much as load a content dialog when there's already one open, we get holes in the terminal window (#12447)
This commit introduces logic to
TerminalPage
to check whether there is a content dialog open before we try to load another one.PR Checklist
Validation Steps Performed
Can no longer repro #12447