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

Feature/menu groups support #10

Closed
wants to merge 2 commits into from
Closed

Feature/menu groups support #10

wants to merge 2 commits into from

Conversation

ilkeraslan
Copy link

@ilkeraslan ilkeraslan commented Oct 11, 2020

This PR addresses the issue #1 .

addToGroup() example:

Screenshot_1602413595

removeFromGroup() example:

Screenshot_1602413616

Note: The setGroupVisible() method is not implemented as it sets the visibility of MenuItems without removing the space. One possible workaround would be to call removeFromGroup() method when the user calls setGroupVisible(false) and addToGroup() method when the user calls setGroupVisible(false). To be able to implement this, though, we should memorize the MenuItems added locally before removing them in order to recreate them when setGroupVisible(false) is called. Consequently, this would be a duplicate implementation of the existing addToGroup() and removeFromGroup() methods.

@saket
Copy link
Owner

saket commented Oct 15, 2020

Thank you @Dement0, but I'll have to this in favor of #9. If you're picking up a feature, it's worth calling dibs on the issue to make sure someone else isn't already working on it 😄.

FWIW I don't think there's enough value in exposing proxy setters for CascadePopupMenu#menu when it's already a public property. This is also in line with PopupMenu.

@saket saket closed this Oct 15, 2020
@ilkeraslan
Copy link
Author

ilkeraslan commented Oct 15, 2020

Thank you @Dement0, but I'll have to this in favor of #9. If you're picking up a feature, it's worth calling dibs on the issue to make sure someone else isn't already working on it smile.

FWIW I don't think there's enough value in exposing proxy setters for CascadePopupMenu#menu when it's already a public property. This is also in line with PopupMenu.

@saket In fact I asked for to work on this issue before #9 (if you check the related issue you can easily notice that) and nobody was working on it so I've made the PR. While #9 was made before asking for if someone was already working on it or not.

For the record, #9 was opened just a few seconds before my PR.

Anyways, it's your project so you decide. Good luck!

@ilkeraslan ilkeraslan deleted the feature/menu_groups_support branch October 15, 2020 19:21
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.

None yet

2 participants