Skip to content

Adding support for grouped menus - #9

Merged
saket merged 3 commits into
saket:trunkfrom
waseefakhtar:feature/menu-groups
Jan 31, 2021
Merged

Adding support for grouped menus#9
saket merged 3 commits into
saket:trunkfrom
waseefakhtar:feature/menu-groups

Conversation

@waseefakhtar

Copy link
Copy Markdown
Contributor

Implementing #1

Demo:

TODO:

  1. setGroupVisible currently hides the group but does not adjust the menu height.
  2. Adding support for setGroupCheckable.
  3. setGroupEnabled is functional but the group state stays the same. (Perhaps we could add color states for disabled items)

Also, it'd be nice to add icons for the sample grouped menu items. I can add them if there's an icon set you've already used for the other items that I can have access to.

@saket saket left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

setGroupVisible currently hides the group but does not adjust the menu height.

Can you explain more? Why does the popup height change?

Adding support for setGroupCheckable

Let's leave this for now

setGroupEnabled is functional but the group state stays the same. (Perhaps we could add color states for disabled items)

What does PopupMenu do here?

Comment thread cascade/src/main/java/me/saket/cascade/CascadeMenuAdapter.kt Outdated
Comment thread cascade/src/main/java/me/saket/cascade/CascadeMenuAdapter.kt Outdated
Comment thread cascade/src/main/java/me/saket/cascade/CascadePopupMenu.kt Outdated
Comment thread cascade/src/main/java/me/saket/cascade/MenuItemViewHolder.kt Outdated
Comment thread sample/src/main/java/me/saket/cascade/sample/MainActivity.kt Outdated
Comment thread sample/src/main/java/me/saket/cascade/sample/MainActivity.kt Outdated
@saket saket mentioned this pull request Oct 15, 2020
@waseefakhtar

waseefakhtar commented Oct 15, 2020

Copy link
Copy Markdown
Contributor Author

setGroupVisible currently hides the group but does not adjust the menu height.

Can you explain more? Why does the popup height change?

This was the same bug as #12, which we resolved in #13.

Adding support for setGroupCheckable

Let's leave this for now

Sounds good.

setGroupEnabled is functional but the group state stays the same. (Perhaps we could add color states for disabled items)

What does PopupMenu do here?

This looks more of an enhancement for styles in sample rather than in cascade.

Setting any item disabled keeps it the same visually. So perhaps we just need to add ColorStateList for text color and icon's tint in cascadeMenuStyler. Not within the scope of this PR, so let me know if you want me to create an issue and work on it on another PR.

@waseefakhtar
waseefakhtar requested a review from saket October 15, 2020 20:07
@nuhkoca

nuhkoca commented Oct 16, 2020

Copy link
Copy Markdown

@saket do you realize the glitch in title change during animation? It seems kinda blinking/clashing. Or has it been changed since the newest commits? You might want to see a new demo. 🤔

@bojankoma

Copy link
Copy Markdown

@saket do you realize the glitch in title change during animation? It seems kinda blinking/clashing. Or has it been changed since the newest commits? You might want to see a new demo. 🤔

I think that glitch comes from Theme.MaterialComponents.

@nuhkoca

nuhkoca commented Oct 16, 2020

Copy link
Copy Markdown

@bojankoma How? Any reason behind this?

@saket

saket commented Oct 16, 2020

Copy link
Copy Markdown
Owner

I assume that's related to #4, which was fixed in v1.1.0. Please file an issue if you're still seeing it?

@nuhkoca

nuhkoca commented Oct 16, 2020

Copy link
Copy Markdown

Okay, I gave the forked repository a try and didn't encounter any weird behavior. It might have been resolved with the last release. Thanks!

@waseefakhtar can you please update the demo gif?

@bojankoma

Copy link
Copy Markdown

@bojankoma How? Any reason behind this?

@nuhkoca I think the animations for enter or exit are different. I didn't go deep into codebase but I already noticed some key differences between using Theme.Appcompat and Theme.MaterialComponents as your app theme.
@saket Please, consider your own bundled anims to get more consistency across the theme engine.

@saket

saket commented Oct 20, 2020

Copy link
Copy Markdown
Owner

@bojankoma Interesting. Possible for you to share a video?

@saket
saket force-pushed the feature/menu-groups branch from 6698d75 to 01514fc Compare January 31, 2021 07:53
@saket

saket commented Jan 31, 2021

Copy link
Copy Markdown
Owner

Sorry it took me way too long to merge this PR. Thanks for contributing @waseefakhtar!

@saket
saket merged commit 4d88bb2 into saket:trunk Jan 31, 2021
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.

4 participants