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.

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

[dynamic_color] Impl. [isCorePaletteSupported] API #390 #481

Closed
wants to merge 4 commits into from
Closed

[dynamic_color] Impl. [isCorePaletteSupported] API #390 #481

wants to merge 4 commits into from

Conversation

alexrintt
Copy link
Contributor

Description

I did need a built-in method to verify if [getCorePalette] is supported by the host platform without third-party solutions such as pub.dev/available or pub.dev/device_info_plus,
see also #390.

Note: this PR includes only the [isCorePaletteSupported] API, I'll try to send another PR to implement [isAccentColorPaletteSupported] also, if/when it gets approved.

Tests

  • Added test cases to check whether or not the method is handling null, false and true values coming from the host platform using the same way it's currently implemented for the other APIs.

Issues

Partially fixes #390 since this PR is missing the [isAccentColorPaletteSupported] which is just the same check as I did in this PR but for the desktop platforms.

Checklist

@alexrintt alexrintt requested a review from a team as a code owner October 13, 2023 16:39
@alexrintt alexrintt requested review from guidezpl and removed request for a team October 13, 2023 16:39
@google-cla
Copy link

google-cla bot commented Oct 13, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

guidezpl
guidezpl requested changes Dec 5, 2023
View reviewed changes
Copy link
Collaborator

@guidezpl guidezpl 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. However I think it would be preferable to add this check as a short-circuit exit to the existing API (getCorePalette), rather than to add a new method. LMK if you'd like to do this

@alexrintt
Copy link
Contributor Author

alexrintt commented Dec 13, 2023 •
edited
Loading

Sure, since we are going to return null when the dynamic color features aren't available this PR doesn't make sense because the plugin is already returning null when the SDK Int is below S.

Also, since for Android there are a few more checks to do in order to determine if this API is available or not, I am planning to open another PR to add these checks in this plugin as well.

In order to do so I was going to add a dependency to com.google.android.material:material and replace Build.VERSION.SDK_INT >= Build.VERSION_CODES.S with DynamicColors.isDynamicColorAvailable() at the getCorePalette verification.

Does that sound reasonable?

@guidezpl
Copy link
Collaborator

Sounds reasonable!

@alexrintt alexrintt closed this Dec 15, 2023
@alexrintt alexrintt mentioned this pull request Dec 16, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guidezpl guidezpl guidezpl requested changes

Assignees
No one assigned
Labels
p: dynamic_color
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

dynamic_color: Add a method to check whether the platform supports dynamic theming
2 participants
@alexrintt @guidezpl