TabView: Add separators and drag/drop support #1113
Conversation
| if (args.DropResult() == winrt::DataPackageOperation::None) | ||
| { | ||
| auto item = args.Items().GetAt(0); | ||
| auto tab = ContainerFromItem(item).try_as<winrt::TabViewItem>(); |
There was a problem hiding this comment.
I prefer locals that save some computation to be defined as const, that way when I see a local used within a scope I can jump to its definition and be sure that I understand its state. Without const I have to check the rest of the code to be sure that there wasn't some if statement that changed the local before the spot I'm interested in got to it. In this case the function is small enough that this isn't a big deal but I still find this patterns a little harder to work with.
I'd suggest doing the computation of the true value of tab within a lambda so that the local within this scope can be const.
There was a problem hiding this comment.
I don't prefer to make lambdas that will only be used in one place, I feel it hinders readability.
There was a problem hiding this comment.
I think lambdas for scoping (even when they're used just once) is a very useful tool that can improve readability. @StephenLPeters when giving this feedback could you recommend a rewrite of the function that you think is more readable? I'll agree that it's not obvious that wrapping this whole thing in a lambda would be simpler... which might argue for a helper function.
There was a problem hiding this comment.
Sure, just did. Helper function would be great too, maybe even better, although I think I like to locality of a lambda a bit more than the readability you get from a helper.
There was a problem hiding this comment.
I do actually use lambdas for scoping in this very file. :) But I disagree that it makes sense here.
|
Is there a current up to date design spec to view - or even a final design comp - you are working towards? Terminal's tabs look very basic - and I have posted ideas for the control myself which use shadow and a current tab indicator to help them stand apart from each other.
|
|
I'd be interested in seeing a screenshot of this |
|
The design is still forthcoming, but hopefully soon! |
| if (args.DropResult() == winrt::DataPackageOperation::None) | ||
| { | ||
| const auto item = args.Items().GetAt(0); | ||
| auto tab = ContainerFromItem(item).try_as<winrt::TabViewItem>(); |
There was a problem hiding this comment.
| auto tab = ContainerFromItem(item).try_as<winrt::TabViewItem>(); | |
| const auto tab = [this, item]() | |
| { | |
| if(const auto tabAsView = ContainerFromItem(item).try_as<winrt::TabViewItem>()) | |
| { | |
| return tabAsView; | |
| } | |
| if(const auto tabAsFE = item.try_as<winrt::FrameworkElement>()) | |
| { | |
| return tabAsFE; | |
| } | |
| const auto numItems = static_cast<int>(Items().Size()); | |
| for (int i = 0; i < numItems; i++) | |
| { | |
| if(const auto tabItem = ContainerFromIndex(i).try_as<winrt::TabViewItem>()) | |
| { | |
| if (tabItem.Content() == item) | |
| { | |
| return tabItem; | |
| } | |
| } | |
| } | |
| return nullptr; | |
| }(); |
There was a problem hiding this comment.
You might have to static_cast each of the returns to get the compiler to be happy that each branch returns the same type =/
| if (args.DropResult() == winrt::DataPackageOperation::None) | ||
| { | ||
| auto item = args.Items().GetAt(0); | ||
| auto tab = ContainerFromItem(item).try_as<winrt::TabViewItem>(); |
There was a problem hiding this comment.
Sure, just did. Helper function would be great too, maybe even better, although I think I like to locality of a lambda a bit more than the readability you get from a helper.
jevansaks
added
the
release note
|
🎉 Handy links: |
I added separators between tabs. The line appears at the right end of each tab item, and is covered up by a -1 margin on the left side by the tab next to it.
I also added the TabDraggedOutside event and pass along other drag/drop events from ListView, and a test for that scenario.