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

fix: Apply icon color correctly inside native AppBarButton #6844

Merged
merged 4 commits into from
Aug 31, 2021

Conversation

Youssef1313
Copy link
Member

@Youssef1313 Youssef1313 commented Aug 23, 2021

GitHub Issue (If applicable): #6732 (Android only is fixed)

PR Type

What kind of change does this PR introduce?

  • Bugfix

What is the current behavior?

Icons inside AppBarButton don't get their color correctly.

What is the new behavior?

Correctly set their color.

PR Checklist

Please check if your PR fulfills the following requirements:

Other information

Internal Issue (If applicable):

@Youssef1313 Youssef1313 requested a review from a team August 23, 2021 20:03
@gitpod-io
Copy link

gitpod-io bot commented Aug 23, 2021

@jeromelaban
Copy link
Member

Don't mind the build errors, it looks like a nuget fluke.

It's important to create a ui test for that one, though. You can create a solid colored square, and poke at the pixels to determine if the change was done properly.

@jeromelaban
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MartinZikmund MartinZikmund added the ready-to-merge Automatically merge the PR once all '.mergify.yml' policies are met label Aug 26, 2021
@Youssef1313
Copy link
Member Author

@MartinZikmund Can you re-run the failing tests? (I don't have permissions to do so)

@jeromelaban
Copy link
Member

The UI test did not work it seems:
image

For wasm it's expected as it's a native command bar test, which may not apply, but for iOS, the test should be working though.

https://dev.azure.com/uno-platform/Uno%20Platform/_TestManagement/Runs?runId=1563684&_a=resultSummary&resultId=100000

Looking at the screenshots, the result does not seem to be correct but this is expected as the XAML is supposed to be running on Android only:

You'll certainly need to create a new sample XAML file to test your scenario all available platforms.

@Youssef1313 Youssef1313 force-pushed the issues/6732 branch 2 times, most recently from 2dab12d to 114cbb8 Compare August 29, 2021 10:06
@Youssef1313
Copy link
Member Author

@jeromelaban This doesn't seem to be working on iOS. I don't have an iPhone or a Mac to try figuring out what's happening. Can we skip the test for iOS for now and open a new issue tracking that?

@jeromelaban
Copy link
Member

You'll need to exclude this test for WebAssembly (the CommandBar is not set for this platform in the test). As for iOS, you can troubleshoot by looking at the screenshots there: https://dev.azure.com/uno-platform/Uno%20Platform/_TestManagement/Runs?runId=1564386&_a=resultSummary&resultId=100000

@Youssef1313
Copy link
Member Author

Yeah I saw the screenshot in CI. For iOS, the icon doesn't seem to appear at all. Hard to investigate without a real device to debug. I reverted the iOS change temporarily to see if the icon will start appearing, after that let's see how we can move forward.

@Youssef1313 Youssef1313 marked this pull request as draft August 30, 2021 15:39
@Youssef1313 Youssef1313 reopened this Aug 30, 2021
@Youssef1313 Youssef1313 reopened this Aug 30, 2021
@Youssef1313 Youssef1313 marked this pull request as ready for review August 30, 2021 19:29
@Youssef1313
Copy link
Member Author

Youssef1313 commented Aug 30, 2021

@jeromelaban I skipped the test for iOS for now since I'm not sure what's going on (I suspect the test XAML code itself is problematic on iOS, since there is no icon at all).

@mergify mergify bot merged commit 4a85f10 into unoplatform:master Aug 31, 2021
@Youssef1313 Youssef1313 deleted the issues/6732 branch August 31, 2021 06:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge Automatically merge the PR once all '.mergify.yml' policies are met
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants