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

Allow customizing default popup style via theme attribute #53

Merged
merged 4 commits into from
Apr 18, 2019
Merged

Allow customizing default popup style via theme attribute #53

merged 4 commits into from
Apr 18, 2019

Conversation

Tunous
Copy link
Contributor

@Tunous Tunous commented Apr 16, 2019

This pull request adds materialPopupStyle attribute which allows customizing the default style that will be used by all of the popups created by this library (unless overridden in the builder via style property)

Example

2019-04-16_18-37-44

Here I've applied the below change. Notice how all of the popups except for these that define custom style have changed to that dark style with dimmed background.

diff --git a/sample/src/main/res/values/styles.xml b/sample/src/main/res/values/styles.xml
index a7db238572786f68142759156494a2229cc7e331..85c90ef297367cb01a3d015365322964d579e1eb 100644
--- a/sample/src/main/res/values/styles.xml
+++ b/sample/src/main/res/values/styles.xml
@@ -13,6 +13,7 @@
         <item name="colorPrimary">@color/lightColorPrimary</item>
         <item name="colorPrimaryDark">@color/lightColorPrimaryDark</item>
         <item name="colorAccent">@color/colorAccent</item>
+        <item name="materialPopupMenuStyle">@style/Widget.MPM.Menu.Dark.Dimmed</item>
     </style>
 
     <style name="AppTheme.Dark" parent="Theme.AppCompat.NoActionBar">

Additional change

I've also cleaned a bit usage of ContextThemeWrapper. Now it's created only once and passed to MaterialRecyclerViewPopupWindow directly as a Context. Also I've removed the context from PopupMenuAdapter and instead made all views inherit from their parent view context which in turn makes them use the context from MaterialRecyclerViewPopupWindow. (Unless I'm somehow mistaken 😏). You can ignore that second commit if you don't want that change. It shouldn't affect the topic of this pull request but I found it related enough to keep here.

Tunous added 3 commits April 16, 2019 18:30
For example the users could set `<item name="materialPopupMenuStyle">@style/Widget.MPM.Menu.Dark</item>` in their theme which would make all popups default to that style instead of `Widget.MPM.Menu`.
*/
var style: Int = R.style.Widget_MPM_Menu
var style: Int? = null
Copy link
Owner

Choose a reason for hiding this comment

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

I'm wondering if it wouldn't be better to use var style: Int = 0 and check against 0 instead. I always find a bit icky to use Int? or a nullable Integer in Java. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me. I'll change that later.

Copy link
Owner

Choose a reason for hiding this comment

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

Cool, I'll review the rest of PR today/tomorrow. I would like to test this a bit more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

Copy link
Owner

@zawadz88 zawadz88 left a comment

Choose a reason for hiding this comment

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

I've checked the rest and it looks good to me! Thanks for cleaning up ContextThemeWrapper stuff as well - looks better now.
So the only change I would ask is that Int? = null -> Int = 0 (plus relevant KDoc/test updated according to that change)

@zawadz88 zawadz88 merged commit d2b03db into zawadz88:master Apr 18, 2019
@Tunous Tunous deleted the feature/default-style branch April 18, 2019 04:39
@zawadz88 zawadz88 mentioned this pull request Apr 30, 2019
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