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

[Evolution] Merged Dictionaries #1138

Merged
merged 3 commits into from Sep 22, 2017

Conversation

Projects
None yet
7 participants
@StephaneDelcroix
Member

StephaneDelcroix commented Sep 13, 2017

NOTE0: this PR replaces #869
NOTE1: Do not squash merge to preserve author information. Thanks

Description of Change

The addition of Multiple MergedDictionaries into ResourceDictionary, as discussed in the Evolution forum: https://forums.xamarin.com/discussion/86308/multiple-merged-dictionaries. Allows multiple ResourceDictionaries to be added to the VisualElement root Resources property.

Bugs Fixed

  • N/A

API Changes

Added:

  • ICollection MergedDictionaries // In ResourceDictionary.cs

Behavioral Changes

The addition, of adding multiple ResourceDictionaries into a ResourceDictionary. No behavioral changes to existing functionality were made.

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of master at time of PR
  • Changes adhere to coding standard
  • Consolidate commits as makes sense

@StephaneDelcroix StephaneDelcroix changed the title from Adamped merged resources dictionaries to [Evolution] Merged Dictionaries Sep 13, 2017

@StephaneDelcroix StephaneDelcroix referenced this pull request Sep 13, 2017

Closed

[Evolution] Merged Dictionaries #869

4 of 4 tasks complete

@StephaneDelcroix StephaneDelcroix requested review from patridge and removed request for patridge Sep 13, 2017

@StephaneDelcroix

This comment has been minimized.

Show comment
Hide comment
@StephaneDelcroix

StephaneDelcroix Sep 13, 2017

Member

@adamped I did some minor fixes here and there, added tests and support for XamlC. give me a thumbs up if you think it's good

Member

StephaneDelcroix commented Sep 13, 2017

@adamped I did some minor fixes here and there, added tests and support for XamlC. give me a thumbs up if you think it's good

@adamped

This comment has been minimized.

Show comment
Hide comment
@adamped

adamped Sep 13, 2017

Contributor

@StephaneDelcroix - why the removal of the Reset event in the CollectionChanged? If the developer, in code behind, for some reason, does this.Resources.MergedDictionaries.Clear();

It then triggers a Reset, then none of the OldItems will have their OnValueChanged event hooks removed, because the OldItems isn't populated on a Reset.

But other than that, all the other changes are good. 👍

Contributor

adamped commented Sep 13, 2017

@StephaneDelcroix - why the removal of the Reset event in the CollectionChanged? If the developer, in code behind, for some reason, does this.Resources.MergedDictionaries.Clear();

It then triggers a Reset, then none of the OldItems will have their OnValueChanged event hooks removed, because the OldItems isn't populated on a Reset.

But other than that, all the other changes are good. 👍

@StephaneDelcroix

This comment has been minimized.

Show comment
Hide comment
@StephaneDelcroix

StephaneDelcroix Sep 14, 2017

Member

@adamped trust your unit tests more. the Reset code is there https://github.com/xamarin/Xamarin.Forms/pull/1138/files#diff-210a3a9d3639265f22acd446c2e1d050R61 (I only moved it up a few lines)

Member

StephaneDelcroix commented Sep 14, 2017

@adamped trust your unit tests more. the Reset code is there https://github.com/xamarin/Xamarin.Forms/pull/1138/files#diff-210a3a9d3639265f22acd446c2e1d050R61 (I only moved it up a few lines)

@adamped

This comment has been minimized.

Show comment
Hide comment
@adamped

adamped Sep 14, 2017

Contributor

@StephaneDelcroix so it is :) All good then.

Contributor

adamped commented Sep 14, 2017

@StephaneDelcroix so it is :) All good then.

@StephaneDelcroix StephaneDelcroix referenced this pull request Sep 19, 2017

Merged

[Xaml/XamlC] better RD inflation #1148

4 of 4 tasks complete

@xamarin xamarin deleted a comment from opcodewriter Sep 20, 2017

adamped and others added some commits May 15, 2017

Type _mergedWith;
[TypeConverter (typeof(TypeTypeConverter))]
public Type MergedWith {

This comment has been minimized.

@jassmith

jassmith Sep 21, 2017

Member

Maybe we should deprecate this?

@jassmith

jassmith Sep 21, 2017

Member

Maybe we should deprecate this?

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix Sep 21, 2017

Member

we will, but we need to implement Source for that

@StephaneDelcroix

StephaneDelcroix Sep 21, 2017

Member

we will, but we need to implement Source for that

@StephaneDelcroix StephaneDelcroix merged commit 712e7f7 into master Sep 22, 2017

16 of 25 checks passed

Android-UITests-Stable Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: UI Tests :: OSX Test Cloud Package - Run Android 6.0.1 : Tests faile…
Details
VSTS: Android API19 Validation Fast Renderers UITests Finished
Details
VSTS: Android API23 Validation Fast Renderers UITests Finished
Details
VSTS: Android API25 Validation Fast Renderers UITests Finished
Details
VSTS: Android API25 Validation Legacy Renderers UITests Finished
Details
VSTS: iOS 11 Validation UITests Finished
Details
iOS10-UITests-Stable Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS10 : Failed …
Details
iOS8-UITests-Stable Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified IOS8 : Failed t…
Details
iOS9-UITests-Stable Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS9 : Failed t…
Details
Android-UITests-Stable-LegacyRenderers Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: UI Tests :: OSX Test Cloud Package - Run Android 6.0.1 (Legacy Rende…
Details
Android-UITests-Stable-PreAppCompat Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: UI Tests :: OSX Test Cloud Package - Run Android 6.0.1 (Pre-AppCompa…
Details
NUPKG-Xamarin.Forms Xamarin.Forms.2.3.6.86887.nupkg
Details
NUPKG-Xamarin.Forms.AppLinks Xamarin.Forms.AppLinks.2.3.6.86887.nupkg
Details
NUPKG-Xamarin.Forms.Maps Xamarin.Forms.Maps.2.3.6.86887.nupkg
Details
NUPKG-Xamarin.Forms.Pages Xamarin.Forms.Pages.2.3.6.86887.nupkg
Details
NUPKG-Xamarin.Forms.Pages.Azure Xamarin.Forms.Pages.Azure.2.3.6.86887.nupkg
Details
OSX-Debug-Stable Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: OSX Debug : Running
Details
VSTS: Android API19 Validation Legacy Renderers UITests Finished
Details
VSTS: Android API23 Validation Legacy Renderers UITests Finished
Details
VSTS: Xamarin Forms OSX pull-1138 - (1006887) Succeeded
Details
VSTS: Xamarin Forms Windows 1006887 Succeeded
Details
VSTS: Xamarin Forms Windows (PR Builds) Started PR process 994567
Details
VSTS: iOS Validation UITests Finished
Details
Windows-Debug-Stable Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: Windows Debug : Tests passed: 3917, ignored: 18
Details
Windows-Release-Stable Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: Windows Release Unit Tests : Tests passed: 3917, ignored: 18
Details

@StephaneDelcroix StephaneDelcroix deleted the adamped-mergedResourcesDictionaries branch Sep 22, 2017

@samhouts samhouts added D-15.5 and removed cla-already-signed labels Oct 18, 2017

@Sokoo92

This comment has been minimized.

Show comment
Hide comment
@Sokoo92

Sokoo92 Mar 1, 2018

Hey guys, loving this feature, I'm using it in my application for brand theming. The only problem I have is that it doesn't work on iOS :( I use a build configuration for each brand in my application.
`#if BRAND1
Resources.MergedDictionaries.Add(new BrandTheme1());
brandName = "Brand1";
#endif

#if BRAND2
Resources.MergedDictionaries.Add(new BrandTheme2());
brandName = "Brand2";
#endif`
And I originally keep my App.xaml resource dictionary empty and Merge the themes I want at build this in my App constructor in App.xaml.cs. But when I run it on iOS it says there's no such key as "Primary" (a primary color in my app). So I thought I'd fill the Dictionary and let the MergedDictionaries override the styles at buildtime. But it doesn't. it keeps the styles I put in App.xaml ResourceDictionary and doesn't merge with the ones I tell it to.

Everything works fine for Android, keeping my App.xaml empty, it merges the correct dictionary at buildtime, just iOS doesn't want to do that for some reason. Anyone knows why?

Sokoo92 commented Mar 1, 2018

Hey guys, loving this feature, I'm using it in my application for brand theming. The only problem I have is that it doesn't work on iOS :( I use a build configuration for each brand in my application.
`#if BRAND1
Resources.MergedDictionaries.Add(new BrandTheme1());
brandName = "Brand1";
#endif

#if BRAND2
Resources.MergedDictionaries.Add(new BrandTheme2());
brandName = "Brand2";
#endif`
And I originally keep my App.xaml resource dictionary empty and Merge the themes I want at build this in my App constructor in App.xaml.cs. But when I run it on iOS it says there's no such key as "Primary" (a primary color in my app). So I thought I'd fill the Dictionary and let the MergedDictionaries override the styles at buildtime. But it doesn't. it keeps the styles I put in App.xaml ResourceDictionary and doesn't merge with the ones I tell it to.

Everything works fine for Android, keeping my App.xaml empty, it merges the correct dictionary at buildtime, just iOS doesn't want to do that for some reason. Anyone knows why?

@StephaneDelcroix

This comment has been minimized.

Show comment
Hide comment
@StephaneDelcroix

StephaneDelcroix Mar 1, 2018

Member

@Sokoo92 please open an issue, with a very small reproduction project.

Member

StephaneDelcroix commented Mar 1, 2018

@Sokoo92 please open an issue, with a very small reproduction project.

@Sokoo92

This comment has been minimized.

Show comment
Hide comment
@Sokoo92

Sokoo92 Mar 7, 2018

@StephaneDelcroix Okay just opened one :) #2037

Sokoo92 commented Mar 7, 2018

@StephaneDelcroix Okay just opened one :) #2037

@samhouts samhouts added this to the 2.5.0 milestone May 5, 2018

@samhouts samhouts modified the milestones: 2.5.0, 2.3.0 Jun 27, 2018

@devmikew

This comment has been minimized.

Show comment
Hide comment
@devmikew

devmikew Aug 6, 2018

what is the syntax of the soure uri for dictionaries located in another assembly?

devmikew commented Aug 6, 2018

what is the syntax of the soure uri for dictionaries located in another assembly?

@StephaneDelcroix

This comment has been minimized.

Show comment
Hide comment
@StephaneDelcroix

StephaneDelcroix Aug 9, 2018

Member

@devmikew none at the moment. see #2660

Member

StephaneDelcroix commented Aug 9, 2018

@devmikew none at the moment. see #2660

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