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

Added UniformCornerRadius to Flipper #2758

Merged
merged 1 commit into from
Jul 5, 2022
Merged

Added UniformCornerRadius to Flipper #2758

merged 1 commit into from
Jul 5, 2022

Conversation

nicolaihenriksen
Copy link
Contributor

Fix for #2689

The new DP is only used in the MaterialDesignCardFlipper style. For the default style, calling code can simply add rounded corners to the content that is put in.

Kind of feels like there there should be a UniformCornerRadius attached property for the Card instead. This would avoid exposing a DP on Flipper.cs that is not used in all styles? Would this go into a CardAssist.cs file or would the attached property go directly in the Card.cs file alongside the "real" DP?

@nicolaihenriksen
Added UniformCornerRadius to Flipper
24020a0 
This DP is only used in the MaterialDesignCardFlipper style. For the default style, calling code can simply add rounded corners to the content that is put in.
@ElieTaillard ElieTaillard requested a review from Keboo July 2, 2022 13:37
@Keboo Keboo added this to the 4.6.0 milestone Jul 5, 2022
@Keboo Keboo added enhancement release notes Items are likely to be highlighted in the release notes. labels Jul 5, 2022
Keboo
Keboo approved these changes Jul 5, 2022
View reviewed changes
Copy link
Member

@Keboo Keboo left a comment

Choose a reason for hiding this comment

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

Great work, thank you!

@Keboo Keboo merged commit ceba17b into MaterialDesignInXAML:master Jul 5, 2022
@nicolaihenriksen nicolaihenriksen deleted the fix2689 branch July 5, 2022 06:03
@nicolaihenriksen
Copy link
Contributor Author

Great work, thank you!

What are your thoughts on the attached property approach? Do you think it is better with a DP that only applies to one of the styles (ie. what this PR includes) as opposed to not having a DP on the Flipper, but instead an attached property that can be applied when using the "card-specific" style?

@Keboo
Copy link
Member

Keboo commented Jul 5, 2022

I think for properties that only sometimes apply, the attached property is preferable to a full DP.

@nicolaihenriksen
Copy link
Contributor Author

nicolaihenriksen commented Jul 5, 2022 •
edited
Loading

I think for properties that only sometimes apply, the attached property is preferable to a full DP.

In that case, shouldn't I have refactored into the attached property approach before this PR was merged? And also, there currently isn't a CardAssist class as far as I can tell, would you put the attached property directly in the Card class (next to the full DP)?

UPDATE: I refactored this to use attached properties in PR #2762

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

@Keboo Keboo Keboo approved these changes

Assignees
No one assigned
Labels
enhancement release notes Items are likely to be highlighted in the release notes.
Projects
None yet
Milestone
4.6.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@nicolaihenriksen @Keboo