KEMBAR78
Add svg to default copied bundle items for mac/ios by J-Swift · Pull Request #4863 · dotnet/maui · GitHub
Skip to content

Add svg to default copied bundle items for mac/ios #4863

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
Merged
merged 2 commits into from
May 3, 2022
Merged

Add svg to default copied bundle items for mac/ios #4863

merged 2 commits into from
May 3, 2022

Conversation

@J-Swift
Copy link
Contributor

@J-Swift J-Swift commented Feb 23, 2022

Description of Change

Implements #4862

Additions made

  • Adds svg to default filetypes that get copied to Asset catalog when using "EnableDefaultMauiItems"

PR Checklist

  • Targets the correct branch
  • Tests are passing (or failures are unrelated)
  • Targets a single property for a single control (or intertwined few properties)
  • Adds the property to the appropriate interface
  • Avoids any changes not essential to the handler property
  • Adds the mapping to the PropertyMapper in the handler
  • Adds the mapping method to the WinUI, Android, iOS, and Standard aspects of the handler
  • Implements the actual property updates (usually in extension methods in the Platform section of Core)
  • Tags ported renderer methods with [PortHandler]
  • Adds an example of the property to the sample project (MainPage)
  • Adds the property to the stub class
  • Implements basic property tests in DeviceTests

Does this PR touch anything that might affect accessibility?

  • Does this PR introduce a new control? (If yes, add an example using SemanticProperties to the SemanticsPage)
  • APIs that modify focusability?
  • APIs that modify any text property on a control?
  • Does this PR modify view nesting or view arrangement in anyway?
  • Is there the smallest possibility that your PR will change accessibility?
  • I'm not sure, please help me

If any of the above checkboxes apply to your PR, then the PR will need to provide testing to demonstrate that accessibility still works.

@dnfadmin
Copy link

dnfadmin commented Feb 23, 2022 •
edited
Loading

CLA assistant check
All CLA requirements met.

@rmarinho rmarinho requested a review from mattleibow February 24, 2022 20:21
@rmarinho
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

mattleibow
mattleibow approved these changes Feb 25, 2022
View reviewed changes
Copy link
Member

@mattleibow mattleibow left a comment

Choose a reason for hiding this comment

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

LGTM but @jonathanpeppers might need to confirm that these are the only locations the extensions are needed. I think they are because you got both targets files.

@mattleibow mattleibow added this to the 6.0.300-preview.14 milestone Feb 25, 2022
jonathanpeppers
jonathanpeppers reviewed Feb 25, 2022
View reviewed changes
.nuspec/Microsoft.Maui.Controls.SingleProject.props Outdated
/>
<ImageAsset
Include="$(iOSProjectFolder)**/*.xcassets/**/*.png;$(iOSProjectFolder)**/*.xcassets/*/*.jpg;$(iOSProjectFolder)**/*.xcassets/**/*.pdf;$(iOSProjectFolder)**/*.xcassets/**/*.json"
Include="$(iOSProjectFolder)**/*.xcassets/**/*.png;$(iOSProjectFolder)**/*.xcassets/*/*.jpg;$(iOSProjectFolder)**/*.xcassets/**/*.pdf;$(iOSProjectFolder)**/*.xcassets/**/*.svg;$(iOSProjectFolder)**/*.xcassets/**/*.json"
Copy link
Member

Choose a reason for hiding this comment

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

@rolfbjarne I was looking at what is done in the iOS targets:

https://github.com/xamarin/xamarin-macios/blob/352a0d818a3ec8d7a7dc754d96150130b7aae62e/dotnet/targets/Microsoft.Sdk.DefaultItems.template.props#L36

Should we actually be doing **\*.xcassets\**\*.* here instead and not listing file extensions?

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 would make sense to not list the file extensions, and do the same as we do in iOS.

We did some testing, and it turned out you can put anything inside an *.xcassets directory (dotnet/macios@a0ad207).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't opened a ticket for this, but upon testing resources in main assemblies vs sub assembles it turns out iOS is overwriting main assembly resources with sub assembly resources. Android (correctly?) layers them together. So, if I have a drawable foo in sub assembly and bar in main assembly, the app sees both foo and bar. If I have the same in .xcassets then the app only sees foo.

Copy link
Member

Choose a reason for hiding this comment

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

Could you open a new issue on that against the macios repo?

@rmarinho
Copy link
Member

Can we rebase this, is this good to be merged @rolfbjarne ?

@jonathanpeppers
Copy link
Member

@rmarinho I think Rolf is saying we should do something else instead of merging this: #4863 (comment)

@rmarinho
Copy link
Member

Ok so can we close this one for now?

mattleibow
mattleibow requested changes Apr 21, 2022
View reviewed changes
Copy link
Member

@mattleibow mattleibow left a comment

Choose a reason for hiding this comment

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

Thanks for this PR, I think we want to fix this, but since it seems iOS supports anything in the folder, could you update your PR to actually include *?

Include="$(iOSProjectFolder)**/*.xcassets/**/*.*"

.nuspec/Microsoft.Maui.Controls.SingleProject.props Outdated
/>
<ImageAsset
Include="$(iOSProjectFolder)**/*.xcassets/**/*.png;$(iOSProjectFolder)**/*.xcassets/*/*.jpg;$(iOSProjectFolder)**/*.xcassets/**/*.pdf;$(iOSProjectFolder)**/*.xcassets/**/*.json"
Include="$(iOSProjectFolder)**/*.xcassets/**/*.png;$(iOSProjectFolder)**/*.xcassets/*/*.jpg;$(iOSProjectFolder)**/*.xcassets/**/*.pdf;$(iOSProjectFolder)**/*.xcassets/**/*.svg;$(iOSProjectFolder)**/*.xcassets/**/*.json"
Copy link
Member

Choose a reason for hiding this comment

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

Could you open a new issue on that against the macios repo?

@rmarinho
Copy link
Member

I think we should close this and open a issue on macios repo

@mattleibow
Copy link
Member

They have done the work in the macos repo, we have to replicate in our platforms folder because we are adjusting the default paths.

@mattleibow
Copy link
Member

I think mobile is mixing up the comments, but this PR is just for the correct inclusions. The issue for macos is about resources overwriting things.

@mattleibow
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@mattleibow
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@mattleibow mattleibow self-assigned this May 3, 2022
@mattleibow mattleibow added area-single-project Splash Screen, Multi-Targeting, MauiFont, MauiImage, MauiAsset, Resizetizer community ✨ Community Contribution labels May 3, 2022
@mattleibow mattleibow added delighter platform/ios platform/macos macOS / Mac Catalyst t/enhancement ☀️ New feature or request labels May 3, 2022
@mattleibow mattleibow enabled auto-merge (squash) May 3, 2022 01:54
@mattleibow mattleibow merged commit f746414 into dotnet:main May 3, 2022
@J-Swift J-Swift mentioned this pull request Aug 19, 2022
Closed
@github-actions github-actions bot locked and limited conversation to collaborators Dec 23, 2023
@samhouts samhouts added the fixed-in-6.0.300-rc.3 Look for this fix in 6.0.300-rc.3! label Aug 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@rolfbjarne rolfbjarne rolfbjarne left review comments

@jonathanpeppers jonathanpeppers jonathanpeppers left review comments

@mattleibow mattleibow mattleibow approved these changes

Assignees

@mattleibow mattleibow

Labels

area-single-project Splash Screen, Multi-Targeting, MauiFont, MauiImage, MauiAsset, Resizetizer community ✨ Community Contribution delighter fixed-in-6.0.300-rc.3 Look for this fix in 6.0.300-rc.3! platform/ios platform/macos macOS / Mac Catalyst t/enhancement ☀️ New feature or request

Projects

No open projects
[ARCHIVED] MAUI Planning
Status: Done

Milestone

6.0.300

Development

Successfully merging this pull request may close these issues.

8 participants

@J-Swift @dnfadmin @rmarinho @jonathanpeppers @mattleibow @rolfbjarne @Redth @samhouts