Skip to content

Conversation

@Avid29
Copy link
Contributor

@Avid29 Avid29 commented Nov 17, 2025

Fixes #746

PR Type

What kind of change does this PR introduce?

What is the current behavior?

There is not orientation options for the Segmented control

What is the new behavior?

Add customization options for a vertical Segmented control

PR Checklist

Please check if your PR fulfills the following requirements:

  • Created a feature/dev branch in your fork (vs. submitting directly from a commit on main)
  • Based off latest main branch of toolkit
  • Tested code with current supported SDKs
  • Tests for the changes have been added (if applicable)
  • Header has been added to all new source files
  • Contains NO breaking changes

Other information

@Avid29 Avid29 marked this pull request as draft November 17, 2025 15:13
@Avid29
Copy link
Contributor Author

Avid29 commented Nov 17, 2025

So far I've added an Orientation property to the EqualPanel. I tested it (see below), but it's not actually implemented anywhere yet.

image

@Avid29
Copy link
Contributor Author

Avid29 commented Nov 17, 2025

Here's an update.
image

Though this does have a slight change in the horizontal style, namely the selected indicator is centered on the full SegmentedItem rather than just the content. Here's what that difference looks like in practice.

Before:
image
After:
image

@Avid29 Avid29 force-pushed the Segmented/VerticalOrientation branch from 24309cb to 4d35ca1 Compare November 17, 2025 17:30
@michael-hawker
Copy link
Member

@niels9001 was there a reason the indicator was centered under the content only vs. the entire content? That'll only matter in Icon + Text Horizontal mode.

@michael-hawker
Copy link
Member

@Avid29 this works perfect!

image

Just slotted right into my VSM with changing the orientation and I was done! 🦙❤️ I'll take a look through the code itself in a bit.

Copy link
Member

@michael-hawker michael-hawker left a comment

Choose a reason for hiding this comment

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

Looks pretty clean to me. @niels9001 @Arlodotexe let me know your thoughts.

Comment on lines 163 to 167
var container = ContainerFromIndex(i) as SegmentedItem;
if (container is null)
continue;

container.UpdateOrientation(Orientation);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
var container = ContainerFromIndex(i) as SegmentedItem;
if (container is null)
continue;
container.UpdateOrientation(Orientation);
if (ContainerFromIndex(i) is SegmentedItem item)
{
item.UpdateOrientation(Orientation);
}

Would this be simpler?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. I have a tendency to over user guard checks...

Comment on lines 5 to 6
using Windows.Media.Devices;

Copy link
Member

Choose a reason for hiding this comment

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

Accidental namespace add?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. Visual Studio really just adds whatever as I type and I often forget to even check 🤷

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I wonder if it's better in VS 2026, I'll have to see as I use it more. It's definitely been a bigger problem in the last year or so than it was in the past.

Comment on lines 24 to +25
RegisterPropertyChangedCallback(SelectedIndexProperty, OnSelectedIndexChanged);
RegisterPropertyChangedCallback(OrientationProperty, OnSelectedIndexChanged);
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to extract the logic in OnSelectedIndexChanged into a SaveInitialIndex method then call it from OnSelectedIndexChanged and OnOrientationChanged?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, wait a minute. That's supposed to call OnOrientationChanged... Yet it does work. Why is that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I see. Because I moved the callback to the DependencyProperty declaration. This line is entirely unnecessary.

Avid29 and others added 2 commits November 19, 2025 02:41
@Arlodotexe Arlodotexe self-requested a review November 24, 2025 17:28
@Avid29 Avid29 marked this pull request as ready for review November 26, 2025 13:57
@Avid29
Copy link
Contributor Author

Avid29 commented Nov 26, 2025

This is now blocking for #752

@niels9001
Copy link
Collaborator

image

Horizontal center alignment of the pill under the label is per design spec. But I'm fine with this change as this would align it with NavigationView too.

@Avid29
Copy link
Contributor Author

Avid29 commented Nov 26, 2025

@niels9001 I believe I could get the pill back under the content by finagling the column layout and visual state setters, but as is now I believe it makes the default style's layout more predictable. It might even be simpler if I adjust the default VisualState (though they're always set, so it wouldn't change any behavior).

Copy link
Collaborator

@niels9001 niels9001 left a comment

Choose a reason for hiding this comment

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

This is looking great - and will be really helpful in creating selectable cards or things like a Store-like NavView :)!

Three things that come to mind:

Image
  1. Long text labels don't fit.. Maybe we should use texttrimming for this so it just ... for the chars that no longer fit?

  2. It feels like we need a bit more spacing vertically (maybe 4 instead of 1) by default to better differentiate what icon belongs to what label. Should we introduce a HorizontalSegmentedItemSpacing and VerticalSegmentedItemSpacing?

  3. Have we tested the vertical orientation with the built-in styles too? Could we add an orientation dropdown to this sample too so it's easier to validate any changes in the future?

image

@Avid29
Copy link
Contributor Author

Avid29 commented Nov 26, 2025

@niels9001 I believe I could get the pill back under the content by finagling the column layout and visual state setters, but as is now I believe it makes the default style's layout more predictable. It might even be simpler if I adjust the default VisualState (though they're always set, so it wouldn't change any behavior).

I looked into it. I'd forgotten that to get the pill to left align properly I had to move it outside of the ContentHolder. This makes it much much harder, since it's no longer a child of the grid separating the icon from the content.

@niels9001
Copy link
Collaborator

image

This also happened.. I think it's expected, and probably up to the developer to not use Stretch in vertical mode, right?

@Avid29
Copy link
Contributor Author

Avid29 commented Nov 26, 2025

Hmm. Yeah, idk what expected behavior is there. You're definitely right that I should add options to more of the samples though.

@Avid29
Copy link
Contributor Author

Avid29 commented Nov 26, 2025

  1. Have we tested the vertical orientation with the built-in styles too? Could we add an orientation dropdown to this sample too so it's easier to validate any changes in the future?

Yes, I did think about this. I explcitly decided that the PivotSegmentedStyle ignores the property, though to be honest it would be quite harmless to make it stack, same as ButtonSegmentedStyle. Here's a screenshot with that sample option added.

image

@Avid29
Copy link
Contributor Author

Avid29 commented Nov 26, 2025

  1. Long text labels don't fit.. Maybe we should use texttrimming for this so it just ... for the chars that no longer fit?

I'd noticed this, and looked into that. The issue is TextTrimming is a TextBlock property not found on the ContentPresenter. I'm not sure how this can be added.

@michael-hawker
Copy link
Member

  1. Have we tested the vertical orientation with the built-in styles too? Could we add an orientation dropdown to this sample too so it's easier to validate any changes in the future?

Yes, I did think about this. I explcitly decided that the PivotSegmentedStyle ignores the property, though to be honest it would be quite harmless to make it stack, same as ButtonSegmentedStyle. Here's a screenshot with that sample option added.

image

Might as well update Pivot Style for completeness? I assume the pill would also go to the left like it does with the default style?

CI failure is due to the failing test (which I think is an external issue, needs confirmation).

@niels9001
Copy link
Collaborator

niels9001 commented Nov 26, 2025

  1. It feels like we need a bit more spacing vertically (maybe 4 instead of 1) by default to better differentiate what icon belongs to what label. Should we introduce a HorizontalSegmentedItemSpacing and VerticalSegmentedItemSpacing?

I fiddled with the margins a bit more, and adjusted which UIElements contain the margins to be more sensible. This is the results. Thoughts? It also fixes the long item's text being cutoff on the left.

image

Looking at your screenshot, it looks like the icon/text label are not correctly center aligned?

image

@Avid29
Copy link
Contributor Author

Avid29 commented Nov 26, 2025

  1. It feels like we need a bit more spacing vertically (maybe 4 instead of 1) by default to better differentiate what icon belongs to what label. Should we introduce a HorizontalSegmentedItemSpacing and VerticalSegmentedItemSpacing?

I fiddled with the margins a bit more, and adjusted which UIElements contain the margins to be more sensible. This is the results. Thoughts? It also fixes the long item's text being cutoff on the left.
image

Looking at your screenshot, it looks like the icon/text label are not correctly center aligned?

Hmm... This is actually really affirming to hear because I had noticed it felt off, but by all means it appears it should be symmetrical according to the code so I assumed I was just wrong. I'll look closer now and see what I can do.

@Avid29
Copy link
Contributor Author

Avid29 commented Nov 26, 2025

Fixed the alignment issues. It was caused by centering the grid on the content, which would misalign the icon by 1 pixel if the content had an odd with.

I also overrode the hover margins when in the vertical state, which I has missed before.

image

@michael-hawker michael-hawker force-pushed the Segmented/VerticalOrientation branch from bb11f05 to b2bf5af Compare December 3, 2025 19:41
@michael-hawker
Copy link
Member

Sorry @Avid29 I should have merged maybe for the main update vs. rebase as you're working on this. Wanted to pick up the fixes for the tests to be more reliable for the CI.

@Avid29
Copy link
Contributor Author

Avid29 commented Dec 3, 2025

No problem 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Segmented] Orientation Property

5 participants