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

chore: Update MediaGallery to Uno.Sdk 5.5 #843

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

eriklimakc
Copy link
Contributor

Issue: #828

Update Sdk to 5.4.5
Some files are using tab and others space for indentation. Changes for all to use space.

@eriklimakc eriklimakc requested review from kazo0 and agneszitte October 4, 2024 12:04
@eriklimakc eriklimakc self-assigned this Oct 4, 2024
@eriklimakc
Copy link
Contributor Author

eriklimakc commented Oct 4, 2024

@agneszitte @kazo0 Should we remove MacOS and Windows from the Platforms folder, launchSettings.json (only Windows), and from <TargetFrameworks>, since this app is Android and iOS only?

@agneszitte
Copy link
Contributor

agneszitte commented Oct 4, 2024

@agneszitte @kazo0 Should we remove MacOS and Windows from the Platforms folder, launchSettings.json (only Windows), and from <TargetFrameworks>, since this app is Android and iOS only?

@eriklimakc I think we can keep only the platforms that are needed here. What do you think @kazo0 ?
Also as the class is supposed to work on Catalyst as well, shouldn't it be possible to keep MacOS ?
I have not checked all the details yet, but I would like your first thoughts on it @eriklimakc please

image

@eriklimakc eriklimakc force-pushed the dev/erli/828-MediaGallerySample branch from 19d039d to d1b9377 Compare October 4, 2024 15:19
@eriklimakc
Copy link
Contributor Author

@eriklimakc I think we can keep only the platforms that are needed here. What do you think @kazo0 ? Also as the class is supposed to work on Catalyst as well, shouldn't it be possible to keep MacOS ? I have not checked all the details yet, but I would like your first thoughts on it @eriklimakc please

@agneszitte Oh yes, I didn't realise that __IOS__ would also include mac. So yes, I think only Windows should be removed then. Right @kazo0?

@agneszitte
Copy link
Contributor

agneszitte commented Oct 4, 2024

@eriklimakc I think we can keep only the platforms that are needed here. What do you think @kazo0 ? Also as the class is supposed to work on Catalyst as well, shouldn't it be possible to keep MacOS ? I have not checked all the details yet, but I would like your first thoughts on it @eriklimakc please

@agneszitte Oh yes, I didn't realise that __IOS__ would also include mac. So yes, I think only Windows should be removed then. Right @kazo0?

@eriklimakc no __IOS__ on this one normally does not include macos/catalyst
cf. https://platform.uno/docs/articles/platform-specific-csharp.html#if-conditionals
image

@eriklimakc
Copy link
Contributor Author

@eriklimakc no __IOS__ on this one normally does not include macos/catalyst cf. https://platform.uno/docs/articles/platform-specific-csharp.html#if-conditionals

@agneszitte That is funny, because when I change that dropdown to M⁠acCatalyst it continues showing the code as enabled:

image

image

While if I choose Windows the code shows as not enabled/ignored:

image

cc @kazo0

@agneszitte
Copy link
Contributor

This PR will need to be rebased with latest and updated to latest 5.5 stable version
Related new issue: #848

@eriklimakc eriklimakc force-pushed the dev/erli/828-MediaGallerySample branch from d1b9377 to 044a0ec Compare November 14, 2024 13:18
@eriklimakc eriklimakc force-pushed the dev/erli/828-MediaGallerySample branch from 044a0ec to 721b434 Compare November 14, 2024 13:27
@eriklimakc eriklimakc changed the title chore: Update MediaGallery to Uno.Sdk 5.4 chore: Update MediaGallery to Uno.Sdk 5.5 Nov 14, 2024
@eriklimakc eriklimakc marked this pull request as ready for review November 14, 2024 13:29
@eriklimakc
Copy link
Contributor Author

Can someone test iOS, please?

@agneszitte @kazo0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants