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

CornerRadius Lightweight styling Resources API #32

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

kikisaints
Copy link

Initial PR for corner radius API spec that covers all of the resource additions being added as part of the 524 proposal from the main repo.

@kikisaints kikisaints requested review from chigy and chrisglein June 11, 2019 16:30
@kikisaints kikisaints requested a review from a team as a code owner June 11, 2019 16:30
active/ThemingCornerRadii/CornerRadiusAPI.md Outdated Show resolved Hide resolved
<Thickness x:Key="ControlThemeCornerRadius">10</Thickness>
<Thickness x:Key="OverlayThemeCornerRadius">10</Thickness>
</Application.Resources>
```
Copy link
Member

Choose a reason for hiding this comment

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

I think it's worth talking about alternatives and their pros/cons. For example, using a Style to target a control type, either directly:

<Button.Flyout>
  <Flyout>
    <Flyout.FlyoutPresenterStyle>
      <Style TargetType="FlyoutPresenter">
        <Setter Property="CornerRadius" Value="6"/>
      </Style>
    </Flyout.FlyoutPresenterStyle>
    <TextBlock Text="Hello World"/>
  </Flyout>
</Button.Flyout>

Or indirectly:

<Style TargetType="Button">
  <Setter Property="CornerRadius" Value="6"/>
</Style>

For some examples of setting CornerRadius, check out the CornerRadius test page

Copy link
Contributor

Choose a reason for hiding this comment

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

@chrisglein, are you suggesting that in addition to the lightweight styling approach, we'll need to update our documentation to also show examples of property setters? If so, I completely agree.

Copy link
Member

Choose a reason for hiding this comment

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

Property setters, explicit Style, and implicit Style. Basically, "Do you want to change CornerRadius? Here are your options. And let us tell you when to use one instead of another".

For example I was pretty easily* able to add rounded corners to an app by creating a bunch of implicit Styles in my App.xaml. We should explain the advantages/disadvantages of that approach.

*well, you'd have to do a style for every single control that accepts CornerRadius, so by "easy" I mean "not complicated, but verbose"

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure the advantage/disadvantages of implicit vs explicit styles except the ability to scope that style to a single instance or not, and how is that better/worse than using a scope/global lightweight styling resource - can you elaborate?

Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if we added the broad style values (control corner radius & overlay corner radius) do we still need the per-control resource keys? Is the ability to add an implicit style enough? How much developer choice is enough choice? (weighed against support cost and performance implications)

If we're following the pattern set before we'd add per-control resource keys. But resource key bloat is a real thing. If we decide that's the right pattern here as well then sure let's do it. But we should have clear reason why we make each of these tools available.

Copy link

Choose a reason for hiding this comment

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

I'm wondering if we added the broad style values (control corner radius & overlay corner radius) do we still need the per-control resource keys? Is the ability to add an implicit style enough? How much developer choice is enough choice? (weighed against support cost and performance implications)

If we're following the pattern set before we'd add per-control resource keys. But resource key bloat is a real thing. If we decide that's the right pattern here as well then sure let's do it. But we should have clear reason why we make each of these tools available.

I think the idea behind both is that you could use the "global" value to change all the controls to match each other in one go - but then you could override that value only on the control you want to not follow the rest of the UI.

I would imaging the local values by default would use the other global values as a Theme or StaticResource

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if we added the broad style values (control corner radius & overlay corner radius) do we still need the per-control resource keys?

I think this is a great question. When we added lightweight styling for colors, the problem we solved was the need to retemplate a control to change, say, its hover color. But CornerRadius is already a property on most? all? controls. So maybe we need resources for corner radii of something like CalendarView, which might have multiple corners that can be adjusted, but we don't need a resource for Button because setting the property, either per instance or using an implicit style, is sufficient.

Copy link
Author

Choose a reason for hiding this comment

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

I'm wondering if we added the broad style values (control corner radius & overlay corner radius) do we still need the per-control resource keys?

I think this is a great question. When we added lightweight styling for colors, the problem we solved was the need to retemplate a control to change, say, its hover color. But CornerRadius is already a property on most? all? controls. So maybe we need resources for corner radii of something like CalendarView, which might have multiple corners that can be adjusted, but we don't need a resource for Button because setting the property, either per instance or using an implicit style, is sufficient.

CornerRadius is a property on all FrameworkElements, I believe. So it exists for every control, however it just doesn't make sense to have one single one for many controls (like the CalendarView example you gave).

So we could introduce a bit of API inconsistency, by having a lot of cornerradius resources for some controls, and none for others, or we could introduce a resource for every control, and finer grain control can be done through the CornerRadius property directly, through the new Lightweight styling resource (which is cleaner when creating separate share-able themes), or though one of our style setter methods.

@chrisglein I agree it is resource bloat, but at this time we're not committed to better styling methodologies.

Copy link
Member

Choose a reason for hiding this comment

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

CornerRadius is a property on all FrameworkElements, I believe. So it exists for every control, however it just doesn't make sense to have one single one for many controls (like the CalendarView example you gave).

It's on Control. Which means it naturally makes its way to most things. That does mean, however, that we have to reconcile this single CornerRadius value with potentially multiple targets (Does it target the track or the handle for a Slider? Does it affect the box for a DatePicker or also the popup?). We have that API inconsistency regardless of whether we add lightweight styling knobs or not. Having resource keyed values for the different types of radii helps address the sub-components but we still have to clarify precedence between that and Control.CornerRadius.

|1| NavigationView | NavigationViewSelectorThemeCornerRadius | NavigationViewItemPresenter *(Top and Left style)* SelectionIndicator (Rectangle) |
|2| Pivot | PivotSelectorThemeCornerRadius | FocusFollower (Rectangle) |
|3| ScrollIndicator | ScrollIndicatorThemeCornerRadius | *tbd* |
|4| ColorPicker | ColorPickerThemeCornerRadius | *tbd* |
Copy link
Member

Choose a reason for hiding this comment

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

This one is a composite of all sorts of controls and its own thing. I've been trying to track the fine grain list of changes in the figma over in this WinUI issue.

Copy link
Author

@kikisaints kikisaints Jun 12, 2019

Choose a reason for hiding this comment

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

Edit: If you're referring to ColorPicker - yes that one has many "moving parts" and needs a separate dev as well as design evaluation.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe @chrisglein is talking about ColorPicker.

active/ThemingCornerRadii/CornerRadiusAPI.md Outdated Show resolved Hide resolved
<Thickness x:Key="ControlThemeCornerRadius">10</Thickness>
<Thickness x:Key="OverlayThemeCornerRadius">10</Thickness>
</Application.Resources>
```
Copy link
Contributor

Choose a reason for hiding this comment

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

@chrisglein, are you suggesting that in addition to the lightweight styling approach, we'll need to update our documentation to also show examples of property setters? If so, I completely agree.

|1| NavigationView | NavigationViewSelectorThemeCornerRadius | NavigationViewItemPresenter *(Top and Left style)* SelectionIndicator (Rectangle) |
|2| Pivot | PivotSelectorThemeCornerRadius | FocusFollower (Rectangle) |
|3| ScrollIndicator | ScrollIndicatorThemeCornerRadius | *tbd* |
|4| ColorPicker | ColorPickerThemeCornerRadius | *tbd* |
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe @chrisglein is talking about ColorPicker.

active/ThemingCornerRadii/CornerRadiusAPI.md Outdated Show resolved Hide resolved
@mdtauk
Copy link

mdtauk commented Jun 12, 2019

Looking at the examples in the spec document - the ToggleSwitch is affected by this CornerRadius. Will this extend to circular items like the new Slider Thumb, the RadioButtons, ProgressRing?

Also will the padding be adjusted to ensure text is not cut off on totally round edges of controls?

@kikisaints
Copy link
Author

Looking at the examples in the spec document - the ToggleSwitch is affected by this CornerRadius. Will this extend to circular items like the new Slider Thumb, the RadioButtons, ProgressRing?

Also will the padding be adjusted to ensure text is not cut off on totally round edges of controls?

I imagine that padding will need to be adjusted and tested for during implementation, so yes 😄

@chigy to confirm, but I believe this will extend to the slider thumb (need to add) and is TBD on the RadioButtons and ProgressRing. We've been doing investigations into improving ProgressRing all up, so I'm hesitant to add it just yet, and I have not seen any changes to RadioButtons as of yet.

@chigy
Copy link
Member

chigy commented Jun 12, 2019

Looking at the examples in the spec document - the ToggleSwitch is affected by this CornerRadius. Will this extend to circular items like the new Slider Thumb, the RadioButtons, ProgressRing?
Also will the padding be adjusted to ensure text is not cut off on totally round edges of controls?

I imagine that padding will need to be adjusted and tested for during implementation, so yes 😄

@chigy to confirm, but I believe this will extend to the slider thumb (need to add) and is TBD on the RadioButtons and ProgressRing. We've been doing investigations into improving ProgressRing all up, so I'm hesitant to add it just yet, and I have not seen any changes to RadioButtons as of yet.

@kikisaints , @mdtauk ,
No, rounded corner spec does not touch ToggleSwitch, RadioButtons nor ProgressRing. Please refer to the controls listed in #524.
Slider thumb is being made to be more round so it will be touched.

Added more corner radius examples/scenarios. Updated the API names to remove the ``Theme`` part of the resource name. Removed ToggleSwitch as a control receiving rounded-ness.
@kikisaints
Copy link
Author

@chigy - okay I'll remove the toggle switch resource then. Are we allowing slider thumb to be easily adjustable, or is it "perma"-rounding?

@mdtauk
Copy link

mdtauk commented Jun 13, 2019

@chigy - okay I'll remove the toggle switch resource then. Are we allowing slider thumb to be easily adjustable, or is it "perma"-rounding?

If you change the Thumb, you also need to change the containing shape. Not to do so would lead to problems like this...
image

@chigy
Copy link
Member

chigy commented Jun 13, 2019

@chigy - okay I'll remove the toggle switch resource then. Are we allowing slider thumb to be easily adjustable, or is it "perma"-rounding?

@kikisaints , let's step back a bit to your proposal (not mine, yours).
Some of your goals might go against you removing some of these controls?

  • Incurring more template debt
  • Promote poor styling behavior/conduct
  • Not future-proof/scale-able for anyone but us

As a platform, do you think it is a good or bad idea to expose these knobs (including toggle switch)?
From design point of view, we wouldn't be recommending changing these values for consistency sake, but do you think developers would want to easily modify those? What's the PROs and CONs on exposing these knobs?

On a side note, I was thinking slider thumb value to be exposed since anytime we change something, I'd like for us to be able to easily go back to previous visual (not because we want that happen, because I want that to be a good rule of thumb for exposing knobs). A bit unique on ToggleSwitch case since we are not changing anything here.

@kikisaints
Copy link
Author

kikisaints commented Jun 13, 2019

@mdtauk Sorry, when I said slider thumb, I was referring to Slider's slider thumb, not ToggleSwitch's. But you're right, if we were to allow the container to be set, we'd want to adjust the thumb relatively to prevent weirdness' that you've shown in your visual.

@chigy I'm in the camp of the more we can allow people the customize our controls' template attributes, the better. However, not including slider thumb in this iteration doesn't mean we can't add it in a future release. My original spec pitch was that I didn't want us to hard-code any values within the template. We're not necessarily incurring more template debt by not touching a control's template, we're just not fixing it either. Fine line, I know.

The pro of allowing app-authors to customize the thumb and container (separately or together) means a more consistent brand on their part, and allows them to work around any corner cases they run into that we may not have been able to preemptively identify.

The con is that allowing a lot more app-author customization can mean Fluent Design inconsistencies across apps. However, a mild mitigation is that we do have generic.xaml which provides all the defaults, so people do have to go out of their way to make it look different.

Edit: I should also clarify: the reason I was asking if we were "perma"-rounding the slider thumb, was because I was going to say we should add a resource for it if it's getting changed at all next release. If it's not then there's a debate to be had about whether or not we should create resource values for controls who aren't getting changed yet.

@chigy
Copy link
Member

chigy commented Jun 13, 2019

@chigy I'm in the camp of the more we can allow people the customize our controls' template attributes, the better. However, not including slider thumb in this iteration doesn't mean we can't add it in a future release. My original spec pitch was that I didn't want us to hard-code any values within the template. We're not necessarily incurring more template debt by not touching a control's template, we're just not fixing it either. Fine line, I know.

@kikisaints , thanks for the clarification. What I'm hearing is that if we had infinite resources, we might do them all but perhaps it does not give us ROI at this point and if customers needs the flexibility, we will add these later?

@mdtauk
Copy link

mdtauk commented Jun 13, 2019

@chigy I'm in the camp of the more we can allow people the customize our controls' template attributes, the better. However, not including slider thumb in this iteration doesn't mean we can't add it in a future release. My original spec pitch was that I didn't want us to hard-code any values within the template. We're not necessarily incurring more template debt by not touching a control's template, we're just not fixing it either. Fine line, I know.

@kikisaints , thanks for the clarification. What I'm hearing is that if we had infinite resources, we might do them all but perhaps it does not give us ROI at this point and if customers needs the flexibility, we will add these later?

At the minimum, you should implement the ThemeResources and Styling properties needed to move the controls to the new designs you decide upon. So the ControlCornerRadius and OverlayCornerRadius (as well as possibly ControlBorderThickness, ListBorderThickness, OverlayOuterBorderThickness properties)

@kikisaints
Copy link
Author

@mdtauk - absolutely, I agree and I believe that's the plan. What I'm proposing is, if there are no new designs for certain controls that could have corner radius designs in the future (we just don't know what they are yet), that we hold off on creating resources for them until we do have designs.

Just like I'd looove to have ControlBorderThickness, OverlayOuterBorderThickness, etc. but we don't have any plans/justifications for those yet, so we're holding off. We may want to do the same for some CornerRadiis on certain controls that aren't getting a makeover in this release.

Added all the feedback from the spec checkpoint review.
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.

5 participants