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

TabView: Fixed many things #1158

Merged
merged 2 commits into from
Aug 14, 2019
Merged

TabView: Fixed many things #1158

merged 2 commits into from
Aug 14, 2019

Conversation

teaP
Copy link
Contributor

@teaP teaP commented Aug 12, 2019

  • Removed CanCloseTabs, per API review
  • Changed Icon to IconSource (from IconElement) -- this also required adding TemplateSettings.
  • Changed handling of Items() so that once ListView loads, I use its Items() collections instead of mine.
  • Drag/Drop now actually works
  • Add/Remove transition animations are back.
  • Tabs now resize properly when added

One test is currently failing -- need Keith's help with that tomorrow.

@teaP teaP requested a review from kmahone August 12, 2019 22:39
@teaP teaP requested a review from a team as a code owner August 12, 2019 22:39
@jevansaks
Copy link
Member

[webhosthidden]

Are these the API review signed-off things? Can we flip these to WUXC_VERSION_MUXONLY now?

Refers to: dev/TabView/TabView.idl:102 in 7545b1c. [](commit_id = 7545b1c, deletion_comment = False)

llongley
llongley requested changes Aug 13, 2019
View reviewed changes
dev/Generated/TabView.properties.cpp Show resolved Hide resolved
dev/TabView/TabView.cpp Show resolved Hide resolved
dev/TabView/TabView.xaml
@@ -420,8 +431,9 @@
MaxWidth="{ThemeResource TabViewItemHeaderIconSize}"
MaxHeight="{ThemeResource TabViewItemHeaderIconSize}"
Margin="{ThemeResource TabViewItemHeaderIconMargin}">
<ContentPresenter x:Name="Icon"
Content="{TemplateBinding Icon}"
<ContentControl x:Name="IconControl"
Copy link
Member

Choose a reason for hiding this comment

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

Something like this would be useful just in general, in terms of a control that you can give an IconSource and it'll put the resulting IconElement into the UI tree. I imagine that, given that this has already gone through API review, now wouldn't be the time to address that, but we should file an issue if we don't already have one saying that we should make a control like that. I don't imagine it'd be a big work item, and it'd be a handy utility. We had one of those in the OS repo, but it was pulled prior to 19H1 due to not having gone through API review in time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Jevan actually started a thread about this, so I'll await the results -- but agreed, it's not very attractive as is.

dev/TabView/TabView.idl

[WUXC_VERSION_PREVIEW]
[webhosthidden]
unsealed runtimeclass TabViewItemTemplateSettings : Windows.UI.Xaml.DependencyObject
Copy link
Member

@llongley llongley Aug 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Just to make sure, this was also put through API review, right? I found out the last time I added a template settings class that they also need to go through API review despite being ultimately just an implementation detail, so I wanted to make sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well... no, admittedly. I didn't know it needed to. I'll email Mike about it then.

Copy link
Member

Choose a reason for hiding this comment

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

Glad I asked, then. :) I didn't know these needed to go through API review either.

dev/TabView/TabView.xaml Show resolved Hide resolved
dev/TabView/TabView.xaml Show resolved Hide resolved
@teaP
Copy link
Contributor Author

teaP commented Aug 13, 2019

[webhosthidden]

Are these the API review signed-off things? Can we flip these to WUXC_VERSION_MUXONLY now?

Refers to: dev/TabView/TabView.idl:102 in 7545b1c. [](commit_id = 7545b1c, deletion_comment = False)

I'll do that in the next commit when I actually change all the names to the API reviewed ones.

@teaP
The new tests @kmahone added a couple days ago are now failing -- he …
0750154 
…will follow up and look into it later this week. Disabling for now.
kmahone
kmahone approved these changes Aug 14, 2019
View reviewed changes
Copy link
Member

@kmahone kmahone left a comment

Choose a reason for hiding this comment

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

:shipit:

@teaP teaP merged commit 296e665 into master Aug 14, 2019
@teaP teaP deleted the user/teaP/TabViewItems branch August 14, 2019 20:02
@jevansaks jevansaks added the release note PR that we want to call out in the next release summary label Aug 14, 2019
@msft-github-bot
Copy link
Collaborator

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

Handy links:

@marcelwgn marcelwgn mentioned this pull request May 21, 2020
Closed
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@llongley llongley llongley approved these changes

@kmahone kmahone kmahone approved these changes

Assignees
No one assigned
Labels
release note PR that we want to call out in the next release summary
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@teaP @jevansaks @msft-github-bot @llongley @kmahone