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

Dev/xygu/20230823/shadow-rework #752

Merged
merged 3 commits into from
Aug 29, 2023
Merged

Dev/xygu/20230823/shadow-rework #752

merged 3 commits into from
Aug 29, 2023

Conversation

Xiaoy312
Copy link
Contributor

@Xiaoy312 Xiaoy312 commented Aug 24, 2023 •
edited

GitHub Issue (If applicable): closes #707

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Refactoring (no functional changes, no api changes)

What is the current behavior?

What is the new behavior?

  • content's background not responsive to theme changes
  • major refactor

PR Checklist

Please check if your PR fulfills the following requirements:

Other information

n/a

@Xiaoy312
Copy link
Contributor Author

Xiaoy312 commented Aug 24, 2023 •
edited

this is just work-in-progress atm, but feel free to point out any issue in the high-level decision

todo:

  • rebase or reapply from main
  • refactor + clean up
  • sanity checks: skia, winui, droid, ios
  • fix/update test
    • fix failed, need to actual debug the test from other means... edit: fixed
  • full neumorphism setup checks: skia done

---
todo:

  • refactor: move corner-radius back onto content
  • sanity checks against neumorphism styles

@Xiaoy312 Xiaoy312 force-pushed the dev/xygu/20230823/shadow-rework branch from b17f4e6 to 8abcbc5 Compare August 24, 2023 19:13
@Xiaoy312 Xiaoy312 force-pushed the dev/xygu/20230823/shadow-rework branch from 8abcbc5 to f633ff0 Compare August 24, 2023 19:17
@github-actions
Copy link

github-actions bot commented Aug 24, 2023 •
edited by unfurl-links bot

Azure Static Web Apps: Your stage site is ready! Visit it here: https://polite-field-01aa64f0f-752.eastus2.azurestaticapps.net

@Xiaoy312 Xiaoy312 force-pushed the dev/xygu/20230823/shadow-rework branch from 44e1ba3 to d5407b6 Compare August 24, 2023 22:30
@github-actions
Copy link

github-actions bot commented Aug 24, 2023 •
edited by unfurl-links bot

Azure Static Web Apps: Your stage site is ready! Visit it here: https://polite-field-01aa64f0f-752.eastus2.azurestaticapps.net

@Xiaoy312 Xiaoy312 marked this pull request as ready for review August 24, 2023 22:52
@Xiaoy312 Xiaoy312 force-pushed the dev/xygu/20230823/shadow-rework branch from d5407b6 to ccbef6e Compare August 24, 2023 22:54
@github-actions
Copy link

github-actions bot commented Aug 24, 2023 •
edited by unfurl-links bot

Azure Static Web Apps: Your stage site is ready! Visit it here: https://polite-field-01aa64f0f-752.eastus2.azurestaticapps.net

@agneszitte agneszitte linked an issue Aug 25, 2023 that may be closed by this pull request
[WinUI/ShadowContainer] ThemeResources used inside ShadowContainer are not changing when switching themes #707
Closed
7 tasks
@Xiaoy312 Xiaoy312 force-pushed the dev/xygu/20230823/shadow-rework branch from ccbef6e to bc6763b Compare August 25, 2023 20:13
@Xiaoy312 Xiaoy312 force-pushed the dev/xygu/20230823/shadow-rework branch from bc6763b to 86087e4 Compare August 25, 2023 22:15
@github-actions
Copy link

github-actions bot commented Aug 25, 2023 •
edited by unfurl-links bot

Azure Static Web Apps: Your stage site is ready! Visit it here: https://polite-field-01aa64f0f-752.eastus2.azurestaticapps.net

1 similar comment
@github-actions
Copy link

github-actions bot commented Aug 25, 2023 •
edited by unfurl-links bot

Azure Static Web Apps: Your stage site is ready! Visit it here: https://polite-field-01aa64f0f-752.eastus2.azurestaticapps.net

roubachof
roubachof requested changes Aug 26, 2023
View reviewed changes
Copy link
Contributor

@roubachof roubachof left a comment

Choose a reason for hiding this comment

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

I'm reviewing from a iPhone, and there are a lot of changes here so I'm only addressing an obvious issue: why remove the detection of child corner radius?

For now we only support children with corner radius, but later we could support text or images. In those cases the path for the shadows will be detected just as we detect children corner radius.
We need to determine the aspect of the shadows from the child.

@Xiaoy312
Copy link
Contributor Author

it all got moved to .ShadowContainer.cs as BindToPaintingProperties
and i refactored it to be cleaner and have all monitors (corner radius, background, shadows) in one place

$class-name.properties.cs should stay pure, nothing more than dependency property definitions

@Xiaoy312 Xiaoy312 force-pushed the dev/xygu/20230823/shadow-rework branch from bc18145 to c8c485f Compare August 26, 2023 16:52
@github-actions
Copy link

github-actions bot commented Aug 26, 2023 •
edited by unfurl-links bot

Azure Static Web Apps: Your stage site is ready! Visit it here: https://polite-field-01aa64f0f-752.eastus2.azurestaticapps.net

@kazo0
Copy link
Contributor

kazo0 commented Aug 27, 2023

it all got moved to .ShadowContainer.cs as BindToPaintingProperties and i refactored it to be cleaner and have all monitors (corner radius, background, shadows) in one place

$class-name.properties.cs should stay pure, nothing more than dependency property definitions

FYI @roubachof ^

Just to make sure you saw the response to your concerns

@Xiaoy312 Xiaoy312 force-pushed the dev/xygu/20230823/shadow-rework branch from c8c485f to ccd12ca Compare August 28, 2023 14:12
@github-actions
Copy link

github-actions bot commented Aug 28, 2023 •
edited by unfurl-links bot

Azure Static Web Apps: Your stage site is ready! Visit it here: https://polite-field-01aa64f0f-752.eastus2.azurestaticapps.net

@Xiaoy312 Xiaoy312 force-pushed the dev/xygu/20230823/shadow-rework branch from ccd12ca to 645f455 Compare August 28, 2023 14:29
@Xiaoy312 Xiaoy312 force-pushed the dev/xygu/20230823/shadow-rework branch from 645f455 to a8e6843 Compare August 28, 2023 14:33
@github-actions
Copy link

github-actions bot commented Aug 28, 2023 •
edited by unfurl-links bot

Azure Static Web Apps: Your stage site is ready! Visit it here: https://polite-field-01aa64f0f-752.eastus2.azurestaticapps.net

1 similar comment
@github-actions
Copy link

github-actions bot commented Aug 28, 2023 •
edited by unfurl-links bot

Azure Static Web Apps: Your stage site is ready! Visit it here: https://polite-field-01aa64f0f-752.eastus2.azurestaticapps.net

@roubachof
Copy link
Contributor

roubachof commented Aug 28, 2023 •
edited

@kazo0 @Xiaoy312 I stand by what I said. Child CornerRadius detection has been removed and the corner radius with this PR is now directly provided by the container itself.
It shouldn't.
The shape of the shadows should be detected from the child.
For now shadows only support rectangular shapes with corner radius, but later it will support Images, and Texts. And the shadows shape will be determined from the shape of the child, just as we are doing now by detecting child corner radius.

@eriklimakc eriklimakc mentioned this pull request Aug 28, 2023
Draft
6 tasks
@Xiaoy312 Xiaoy312 force-pushed the dev/xygu/20230823/shadow-rework branch 3 times, most recently from 1cd6cd7 to 823ab6a Compare August 29, 2023 03:02
@Xiaoy312 Xiaoy312 force-pushed the dev/xygu/20230823/shadow-rework branch from 823ab6a to b57d6e6 Compare August 29, 2023 03:23
@github-actions
Copy link

github-actions bot commented Aug 29, 2023 •
edited by unfurl-links bot

Azure Static Web Apps: Your stage site is ready! Visit it here: https://polite-field-01aa64f0f-752.eastus2.azurestaticapps.net

@agneszitte
Copy link
Contributor

agneszitte commented Aug 29, 2023 •
edited

@Xiaoy312 / @kazo0 / @jeromelaban
None of these examples use proper ThemeResources so we can test the theme switching colors (light <-> dark) in the toolkit sample app (can be done in a separate following PR)
cf. resources here:

uno.toolkit.ui/samples/Uno.Toolkit.Samples/Uno.Toolkit.Samples.Shared/Content/Controls/ShadowContainerSamplePage.WinUI.xaml

Line 15 in f760d54

<Color x:Key="UnoColor">#7a67f8</Color>

image

@jeromelaban
Copy link
Member

None of these examples use proper ThemeResources so we can test the theme switching colors (light <-> dark) in the toolkit sample app (can be done in a separate following PR)

@agneszitte if it's not impacting the gallery, we can do it in a separate PR

@agneszitte
Copy link
Contributor

agneszitte commented Aug 29, 2023 •
edited

@Xiaoy312 some runtime tests would also need to be added in a separated PR, please

@jeromelaban jeromelaban merged commit ce2a55f into main Aug 29, 2023
24 checks passed
@jeromelaban jeromelaban deleted the dev/xygu/20230823/shadow-rework branch August 29, 2023 16:16
@agneszitte
Copy link
Contributor

@Mergifyio backport legacy/3x

@mergify
Copy link
Contributor

mergify bot commented Aug 29, 2023 •
edited

backport legacy/3x

✅ Backports have been created

@mergify mergify bot mentioned this pull request Aug 29, 2023
Merged
@agneszitte agneszitte mentioned this pull request Sep 5, 2023
Closed
@Xiaoy312 Xiaoy312 mentioned this pull request Sep 5, 2023
Merged
18 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pictos pictos pictos left review comments

@kazo0 kazo0 kazo0 approved these changes

@rafael-rosa-knowcode rafael-rosa-knowcode Awaiting requested review from rafael-rosa-knowcode

@jeromelaban jeromelaban Awaiting requested review from jeromelaban

@eriklimakc eriklimakc Awaiting requested review from eriklimakc

@carldebilly carldebilly Awaiting requested review from carldebilly

@roubachof roubachof Awaiting requested review from roubachof

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[WinUI/ShadowContainer] ThemeResources used inside ShadowContainer are not changing when switching themes
6 participants
@Xiaoy312 @kazo0 @roubachof @agneszitte @jeromelaban @pictos