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

Adding ColorPicker #241

Merged
merged 32 commits into from
Mar 5, 2024
Merged

Adding ColorPicker #241

merged 32 commits into from
Mar 5, 2024

Conversation

niels9001
Copy link
Collaborator

@niels9001 niels9001 commented Sep 23, 2023

This PR ports over ColorPicker and ColorPickerButton from: CommunityToolkit/WindowsCommunityToolkit#4502

Changes introduced:

  • New Fluent look and feel
  • Using Segmented control instead of a ListBox
  • The Segmented control will now hide if only 1 color option is selected.
  • Moved the preview color selection to the top of the control
  • New samples
  • Tested on UWP/WASDK - nothing is showing on WASM and no errors (@michael-hawker @Arlodotexe is that something we can park for now and create an issue for?)

ColorPicker
ColorPickervNext2

ColorPickerButton
image

Closes: #69

niels9001 and others added 6 commits September 20, 2023 11:55
Co-Authored-By: robloo <17993847+robloo@users.noreply.github.com>
Co-Authored-By: robloo <17993847+robloo@users.noreply.github.com>
@niels9001
Copy link
Collaborator Author

@Arlodotexe Could you help out with the following:

  • I'm suppressing a couple of warnings as they didn't show up in the error log, only on launch?
  • There's a binding error happening on the inner GridView - mentioned here as well: Port ColorPicker #69 (comment)

niels9001 and others added 3 commits September 23, 2023 16:38
Co-Authored-By: robloo <17993847+robloo@users.noreply.github.com>
Co-Authored-By: robloo <17993847+robloo@users.noreply.github.com>
Co-Authored-By: robloo <17993847+robloo@users.noreply.github.com>
@niels9001
Copy link
Collaborator Author

@Arlodotexe Would you mind looking at what the error is and review?

@niels9001
Copy link
Collaborator Author

@Arlodotexe Would you mind looking at what the error is and review?

@Arlodotexe Ping

@michael-hawker michael-hawker added this to the 8.1 milestone Jan 9, 2024
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.

Will try this out soon. Couple of minor comments/remarks.

@niels9001 did notice a bit of grid illusion though with the new setup?

Co-authored-by: Michael Hawker MSFT (XAML Llama) <24302614+michael-hawker@users.noreply.github.com>
@hawkerm
Copy link

hawkerm commented Jan 14, 2024

Hmm, seem to have some trouble using the nuget package from the pull request feed:

2>C:\Program Files\Microsoft Visual Studio\2022\Community\MSBuild\Current\Bin\amd64\Microsoft.Common.CurrentVersion.targets(5198,5): error MSB3030: Could not copy the file ".nuget\packages\communitytoolkit.uwp.colorpicker\8.1.240110-pull-241.818\lib\uap10.0.17763\CommunityToolkit.WinUI.Controls.Segmented\CommunityToolkit.WinUI.Controls.Segmented.xr.xml" because it was not found.

Not sure why it's looking there instead of the segmented package itself, as that's where it should be, I do see the segmented package as a proper dependency in Nuget package explorer... so bit confused.

@Arlodotexe
Copy link
Member

Arlodotexe commented Jan 18, 2024

Need to remove CornerRadius on the color swatch aswell

@Arlodotexe
Copy link
Member

Found the reason why the component isn't showing on WebAssembly. We're referencing the Animations component for ImplicitAnimationSet, but implicit animations use Composition, which aren't implemented on Uno yet.

image

Our options are to either:

  • Add conditional XAML everywhere ImplicitAnimationSet is used, or
  • Update ImplicitAnimationSet to avoid calling Composition APIs when running under Uno until support is added.

@niels9001 @michael-hawker What are you thoughts?

@michael-hawker
Copy link
Member

Our options are to either:

  • Add conditional XAML everywhere ImplicitAnimationSet is used, or
  • Update ImplicitAnimationSet to avoid calling Composition APIs when running under Uno until support is added.

@niels9001 @michael-hawker What are you thoughts?

Can we combine conditionals with a namespace like that in Uno?

It may be easier to just modify the ImplicitAnimationSet for Uno, then stuff can just work later without needing to change anything if/when it's supported in the future?

We do need a better way of tracking what's not supported in Uno though doing it that way, right? As it's not like we'd have it throw an unsupported exception.

@Arlodotexe
Copy link
Member

Arlodotexe commented Jan 18, 2024

Can we combine conditionals with a namespace like that in Uno?

Official Uno docs recommend wrapping it in a container such as a Grid. That requires adjusting the Visual Tree in a lot of different places, both in our codebase and in the codebase of those consuming our packages

We do need a better way of tracking what's not supported in Uno though doing it that way, right? As it's not like we'd have it throw an unsupported exception.

At a high level, our custom MultiTarget system is designed to mitigate this happening on accident by allowing us to isolate code that depends on unsupported APIs (e.g. Composition) into separate libraries (e.g. Media) and disabling Uno-specific MultiTargets so they can't be used without first adding support and checking/addressing the original reason for lack of support.

When we enable Uno for a component that didn't previously support it, we need to be doing a sweep of the component to make sure nothing throws when running under the new platform. We didn't originally support the wasm MultiTarget for Animations, but we added *partial* support for running on Uno through the FrameworkLayer abstraction, though this support only extended to AnimationBuilder and CustomAnimation.

We can disable implicit animations from running to get this out, but we should also consider a full sweep of the Animations package to see what can be implemented without using Composition, versus what needs to be disabled so it doesn't throw while we wait for Uno to implement Composition.

Giving the Animations package a quick once-through, I've also noted that:

  • CustomAnimation isn't being used by our inbox animations (TranslationAnimation, ScaleAnimation, etc) despite inheriting the same time. They won't default to FrameworkLayer.Xaml on Uno.
  • Implicit/Explicit animations and helpers are still using Composition APIs directly without handling what to do when running under the added Uno tfm

@michael-hawker
Copy link
Member

Yeah, I'd want to avoid changing the visual tree structure specifically for Uno. Makes it harder to undo later when things are supported, and also hurts the performance for everything else otherwise.

@michael-hawker
Copy link
Member

@Arlodotexe still not working with UWP. Tried the latest build, same error message.

@niels9001
Copy link
Collaborator Author

Added a fixed height and wrapped the ColorSpectrum in a ViewBox so that it now nicely takes up available space whenever the sliders are not visible.

ColorPicker

@michael-hawker
Copy link
Member

Attaching UWP binlog from the CI here (compared to the consuming app above):
wct-uwp-colorpicker-segmented-pri-issue.zip

@michael-hawker
Copy link
Member

@Arlodotexe just adding here as we discovered in our last call: https://github.com/CommunityToolkit/WindowsCommunityToolkit/blob/6bfe8a5d21feb8818a8048b9fc41a84a70af8b0f/Microsoft.Toolkit.Uwp.UI.Controls.Markdown/Microsoft.Toolkit.Uwp.UI.Controls.Markdown.csproj#L27-L40

  <!--
    HACK: Fix the 'ProjectReference' inclusion of duplicate UWP resources.
    The UWP project system includes the 'Controls.Core' project's resources because
    it doesn't know, it'll be an independent package later during packing.
    Therefore, we need to remove these extra resources in the packaging pipeline.
  -->
  <Target Name="_RemoveUnwantedResources" AfterTargets="GetPackagingOutputs">
    <!--<Message Text="Package Files Before: @(PackagingOutputs)" Importance="High" />-->
    <ItemGroup>
      <PackagingOutputs Remove="@(PackagingOutputs)" Condition="'%(PackagingOutputs.Filename)%(PackagingOutputs.Extension)' == 'Microsoft.Toolkit.Uwp.UI.Controls.Core.pri'" />
      <PackagingOutputs Remove="@(PackagingOutputs)" Condition="$([System.String]::new('%(PackagingOutputs.TargetPath)').StartsWith('Microsoft.Toolkit.Uwp.UI.Controls.Core\'))" />
    </ItemGroup>
    <!--<Message Text="Package Files After: @(PackagingOutputs)" Importance="High" />-->
  </Target>

For now, can we just try adding this into the csproj for the ColorPicker and see if it resolves the issue? Then we can figure out if we want to generalize later.

Seems like this is a UWP only issue, so we should condition the Target to be UWP only.

@Arlodotexe
Copy link
Member

@michael-hawker Gave this a try and no dice. The reason might be that the Segmented pri isn't included in the nuget package. We might have to craft a custom fix for this.

@michael-hawker
Copy link
Member

@Arlodotexe thanks, I doubt the updates for .NET 8 will fix our underlying issue, can we try the suggestion from the platform team of adding:

<IncludePriFilesOutputGroup>false</IncludePriFilesOutputGroup>

to the ColorPicker csproj?

@Arlodotexe
Copy link
Member

@Arlodotexe thanks, I doubt the updates for .NET 8 will fix our underlying issue, can we try the suggestion from the platform team of adding:

<IncludePriFilesOutputGroup>false</IncludePriFilesOutputGroup>

to the ColorPicker csproj?

This had no effect on the error message we were seeing when the ColorPicker package is built and consumed on UWP.

@michael-hawker
Copy link
Member

@Arlodotexe new fix from the platform folks to try in the ColorPicker library project:

<Target Name="BeforeGenerateProjectPriFile">
  <ItemGroup>
    <_LayoutFile Remove="@(_LayoutFile)" Condition="$([System.String]::Copy(&quot;%(_LayoutFile.TargetPath)&quot;).StartsWith('CommunityToolkit.WinUI.Controls.Segmented\'))" />
  </ItemGroup>
</Target>

Originally, it happened because ColorPicker started using Segmented project, which is a control in its own, which means whole infrastructure around building a library layout assumes that you want everything Segmented control needs at runtime in ColorPicker package.

Deeper down, we can say that root cause is that we conflated concept of "reference assemblies" and "runtime assemblies".

@Arlodotexe
Copy link
Member

@Arlodotexe new fix from the platform folks to try in the ColorPicker library project:

<Target Name="BeforeGenerateProjectPriFile">
  <ItemGroup>
    <_LayoutFile Remove="@(_LayoutFile)" Condition="$([System.String]::Copy(&quot;%(_LayoutFile.TargetPath)&quot;).StartsWith('CommunityToolkit.WinUI.Controls.Segmented\'))" />
  </ItemGroup>
</Target>

Originally, it happened because ColorPicker started using Segmented project, which is a control in its own, which means whole infrastructure around building a library layout assumes that you want everything Segmented control needs at runtime in ColorPicker package.
Deeper down, we can say that root cause is that we conflated concept of "reference assemblies" and "runtime assemblies".

Had to install WinUI 2.7 directly in the test app, but this did the trick!

image

ColorPicker for UWP can now be safely consumed from the NuGet package 🎉

@Arlodotexe Arlodotexe enabled auto-merge (rebase) March 5, 2024 20:12
@Arlodotexe Arlodotexe merged commit 7c3a552 into main Mar 5, 2024
9 checks passed
@delete-merged-branch delete-merged-branch bot deleted the niels9001/colorpicker branch March 5, 2024 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Port ColorPicker
4 participants