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

Added documentation for optional color configurations on Android. #1566

Conversation

bbusenius
Copy link
Contributor

This PR is to add documentation for the new color settings added to briefcase-android-gradle-template in PR #77. The new color settings are:

accent_color
primary_color
primary_color_dark

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

@bbusenius
Copy link
Contributor Author

I took a stab at writing a newsfragment. I hope that wasn't too audacious. I wasn't sure if that minuscule work should rise to the level of a feature but since it's new functionality, that was my best guess. Let me know if you want me to change anything.

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

Looks good! This definitely qualifies as a "feature" to my mind - it's a notable capability that now exists that didn't previously.

A couple of review suggestions inline; it's probably also worth moving the Android-specific notes to the Android configuration guide. That document already mentions splash_background_color; it would make sense to me to expand that section to clarify how the other app colors will be applied.

@@ -0,0 +1 @@
Briefcase Android Gradle Template now allows for setting accent_color, primary_color and primary_color_dark. These settings can now be used in pyproject.toml for Android projects.
Copy link
Member

Choose a reason for hiding this comment

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

Even though it's currently Android-only, the feature is a generic configuration item, so it should be framed in those terms.

Suggested change
Briefcase Android Gradle Template now allows for setting accent_color, primary_color and primary_color_dark. These settings can now be used in pyproject.toml for Android projects.
Apps can now specify a primary color (for both light and dark modes), and an accent color. If the platform allows apps to customize color use, these colors will be used to style the app's presentation.

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 see, yes, that makes perfect sense.

Comment on lines 305 to 308
A hexadecimal RGB color value (e.g., ``#008577``) to use as the primary color
for the application. On Android this will be used to color the app bar in the
main window. This setting is only used if the platform allows color
modification, otherwise it is ignored.
Copy link
Member

Choose a reason for hiding this comment

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

Notes about the specific platform interpretations should be in the relevant platform guide, unless they're indicative of behavior on all platforms.

Suggested change
A hexadecimal RGB color value (e.g., ``#008577``) to use as the primary color
for the application. On Android this will be used to color the app bar in the
main window. This setting is only used if the platform allows color
modification, otherwise it is ignored.
A hexadecimal RGB color value (e.g., ``#008577``) to use as the primary color
for the application. This setting is only used if the platform allows color
modification, otherwise it is ignored.

Comment on lines 313 to 316
A hexadecimal RGB color value (e.g., ``#008577``) used alongside the primary
color. On Android this is used to color the status bar at the top of the
screen. This setting is only used if the platform allows color modification,
otherwise it is ignored.
Copy link
Member

Choose a reason for hiding this comment

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

As above

Suggested change
A hexadecimal RGB color value (e.g., ``#008577``) used alongside the primary
color. On Android this is used to color the status bar at the top of the
screen. This setting is only used if the platform allows color modification,
otherwise it is ignored.
A hexadecimal RGB color value (e.g., ``#008577``) used alongside the primary
color. This setting is only used if the platform allows color modification,
otherwise it is ignored.

@bbusenius
Copy link
Contributor Author

That all makes sense. I made the suggested changes and moved the Android specific language to the Android configuration section of docs/reference/platforms/android/gradle.rst. Feel free to let me know if there's anything else you want me to change.

----------------------
The dark primary color is used alongside the primary color to color the
status bar at the top of the screen.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry - I should have been clearer; this section is for settings that are unique to Android configuration. I was referring to adding a note up around L100, where there's already a mention of how splash_background_color will be interpreted.

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 see it now. Your instructions were fine. I think it just threw me that it was in the Splash Image format section. I created a new section there for colors and added another mention of splash_background_color to be consistent and comprehensive. Though it's a little redundant, I think it makes sense. What do you think?

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

Almost there! A couple more revisions; also - if we're adding a whole section about configurating color (which I agree is a good idea), we can remove the reference to the splash background on L100-101.

Comment on lines 108 to 110
In addition to ``splash_background_color``, Android projects allow for
configuration of an ``accent_color``, ``primary_color``, and
``primary_color_dark``.
Copy link
Member

Choose a reason for hiding this comment

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

There's no need to list all the things that can be configured just before you list all the things that can be configured :-)

Suggested change
In addition to ``splash_background_color``, Android projects allow for
configuration of an ``accent_color``, ``primary_color``, and
``primary_color_dark``.
Android allows for some customization of the colors used by your app:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point! :-)

Comment on lines 112 to 113
* ``accent_color`` is used subtly throughout an Android app to call attention
to key elements. It's used on things like form labels and inputs.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* ``accent_color`` is used subtly throughout an Android app to call attention
to key elements. It's used on things like form labels and inputs.
* ``accent_color`` is used as a subtle highlight throughout your app to call attention
to key elements. It's used on things like form labels and inputs.

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 made all those changes. Thanks for helping me get this in shape!

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks for the patch, and for persisting through all the nitpicking edits!

@freakboy3742 freakboy3742 merged commit 9b389e2 into beeware:main Dec 8, 2023
44 checks passed
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.

2 participants