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

fixed the closed button displaying bug #1580

Merged
merged 2 commits into from
Dec 3, 2019
Merged

Conversation

CipherPol9
Copy link
Contributor

Description

Added UpdateCloseButton() under TabViewItem::OnApplyTemplate()

Motivation and Context

fixes the issue of the close button showing up on the video tab because the TabViewItem was depending on the onloaded event handler to set the close button visibility but this would fail if the template hasn't been applied yet. Fixes #1416

How Has This Been Tested?

Manually

Screenshots (if appropriate):

@msftclas
Copy link

msftclas commented Nov 10, 2019

CLA assistant check
All CLA requirements met.

@StephenLPeters
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@StephenLPeters
Copy link
Contributor

UpdateCloseButton();

I think that the UpdateCloseButton() call in OnApplyTemplate() might cover all the scenarios we are interested in. Can you try deleting this OnLoaded event handler and seeing if it still behaves as intended?


Refers to: dev/TabView/TabViewItem.cpp:113 in c003f69. [](commit_id = c003f69, deletion_comment = False)

@StephenLPeters
Copy link
Contributor

Adding a new test to ensure this functionality would be great

@StephenLPeters
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

protected async override void OnNavigatedTo(Windows.UI.Xaml.Navigation.NavigationEventArgs args)
{
NotCloseableTab.Visibility = Visibility.Collapsed;
await Task.Delay(1);
Copy link
Member

Choose a reason for hiding this comment

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

Please use an override that takes a TimeSpan to make this more readable (so I don't have to go to the docs to see what this does 🙂).

Suggested change
await Task.Delay(1);
await Task.Delay(TimeSpan.FromMilliseconds(1));

@jevansaks jevansaks added the needs-author-feedback Asked author to supply more information. label Nov 15, 2019
@jevansaks
Copy link
Member

@CipherPol9 do you think you'll be able to incorporate these changes?

@wbokkers
Copy link

Any change of this PR to be changed and reviewed? There's only a minor change needed in the test code (and not in the fix itself).

@StephenLPeters
Copy link
Contributor

Any change of this PR to be changed and reviewed? There's only a minor change needed in the test code (and not in the fix itself).

I asked for a product code change, and a new test would be great. @CipherPol9 Do you think you'll be able to address the feedback? If not we should take over this change. @teaP have you taken a look?

@StephenLPeters
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@StephenLPeters
Copy link
Contributor

@jevansaks @teaP maybe take another look with my amendment?

@jevansaks jevansaks merged commit a70f3cf into microsoft:master Dec 3, 2019
@jevansaks jevansaks added area-TabView team-Controls Issue for the Controls team needs-cherrypicktorelease PR tagged for cherry-pick to the current release branch (but not yet picked) and removed needs-author-feedback Asked author to supply more information. labels Dec 3, 2019
@jevansaks jevansaks removed the needs-cherrypicktorelease PR tagged for cherry-pick to the current release branch (but not yet picked) label Dec 11, 2019
@msft-github-bot
Copy link
Collaborator

🎉Microsoft.UI.Xaml v2.3.191211002 has been released which incorporates this pull request.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-TabView team-Controls Issue for the Controls team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Close button in TabViewItem becomes visible on changing Visibility property
7 participants