-
Notifications
You must be signed in to change notification settings - Fork 79
MarqueeText conversion to broad Marquee control #732
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
Conversation
…ntentControl base class
| #pragma warning disable CA1001 | ||
| #endif | ||
| public partial class MarqueeText : Control | ||
| public partial class Marquee : ContentControl |
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.
Sorry, late to the party, would MarqueeView or MarqueePresenter be a better name here? Maybe even MarqueeBox?
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.
You're right, I didn't give the name enough thought when I changed it from MarqueeText. MarqueePresenter sounds best to me, since it serves a similar purpose to the ContentPresenter or SwitchPresenter but as a Marquee.
MarqueeView feels to me like it would be related to the ListView/GridView or NavigationView, which it isn't. Meanwhile MarqueeBox is the same but to the ViewBox or ComboBox.
I can go ahead and add this change to the state transitions PR if you suggest.
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.
Yeah, sounds great, thanks!
We're just about to get the initial push of controls to NuGet working, so will be nice to have a single named package there. 🙂
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.
(I had suggested Box as we have the ContrainedBox as well for aspect ratio handling, but Presenter should be good as it does take a template/content to make up the visuals vs. a box which is a lighter-weight modification of the layout generally. And this is a bit more than that now. Other option would be just MarqueeControl, but that's a bit redundant and a shortening from ContentControl which would add a mouthful if it were MarqueeContentControl.
MarqueePresenter seems good though, can it inherit from ContentPresenter as well or does it need ContentControl?)
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.
I'm not sure that this aligns well with standard XAML naming practices.
ContentControl and ContentPresenter are tightly coupled. They both have ContentTemplate and a template selector prop, but the Marquee controls derives from ContentControl, and we even use ContentPresenter as intended inside the template.
The term has an existing functional meaning via ContentPresenter's relationship to ContentControl, and we're even leveraging it to build this control. Feels off to just call our control a Presenter without acknowledging that, especially knowing new devs will be using these and they won't be getting that consistency with the platform.
Restated more clearly by Copilot (gpt5):
In XAML, Presenter has a very specific, lightweight meaning (ContentPresenter, ItemsPresenter). It signals “I’m a slot inside a template, not a full control.” A ContentControl subclass is the opposite: it’s a host with its own lifecycle and ControlTemplate. Calling it MarqueePresenter would blur that line.
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.
They're not completely tightly coupled, there are other Presenter types (at least between WPF and our own SwitchPresenter).
If it didn't need a template and could work on the content directly, then I'd say Box is the way to go as it's just modifying the presentation/layout of what it's wrapping like Viewbox and ConstrainedBox do.
But since we're in template land we either want it to be a 'light-weight' presenter or we call it a control. I mean we do have the LayoutTransformControl already as another sort of example in a similar space.
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.
If I'm not mistaken, ContentPresenter is really only to be used for presenting the content of a ContentControl (or any object, DataTemplate/DataTemplateSelector pair).
The Marquee is templated, and I believe the Template property comes from the Control class.
Looking at Arlos comment, I think I'm actually leaning towards either MarqueeControl or simply Marquee. The only info added by appending "Control" is to denote that the UI element is templated.
Additionally, to reflect on my previous comment, I'd need to check the implementation, but I presume the SwitchPresenter is actually not templated, and therefore is probably implemented as an extension of ContentPresenter. Or if it is not, it should be.
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.
Discussion continued in a thread in the #community-toolkit channel of the Windows App Community Discord.
Replaces the fixed Text property used for the content of the
MarqueeTextcontrol with a ContentPresenter capable of using templating to achieve the same behavior with any UIElement via templating. The control is also renamed to simplyMarquee.