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

Update menu.ts #71

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

thebluepotato
Copy link

  • Adds an optional event handler to determine whether the item, while shown, is disabled
  • Adds support for registering an element when the popup has no children (for instance, was created with ztoolkit.UI.createElement() just before);

Fixes #70

@windingwind
Copy link
Owner

Thanks for the contribution. Actually the disabled or any attribute could be modified in the hook getVisibility. As we are adding an isDisabled, we'd better:

  1. Unify their names. To me, the name isDisabled sounds like more of a static attribute getter, instead of a source of data.
  2. Or, merge them into one onPopupShowing and let the user decide what to do
  3. Or, add hooks for other possible attributes, e.g. checked.

@thebluepotato
Copy link
Author

To the extent that these are callbacks, isDisabled and isVisible sound like a React state to me. Like the item asking "am I visible?". The alternative being to align with the existing getVisibility would be getDisability (!) or getDisabledStatus. If they're unified, they should probably align with each other, meaning that it should be "isHidden" and "isDisabled" or "isVisible" and "isEnabled" IMO.

A more general onPopupShowing might be nice too. However, what's nice about these specific handlers is that they avoid lots of boilerplate, whereas a general onPopupShowing would force us to call setAttribute every time. I think a mix of 1 and 2 would be best:

  • Unify the names for those two rather common use cases (hidden or not, disabled or not)
  • Add a more general onPopupShowing option

@windingwind
Copy link
Owner

I think a mix of 1 and 2 would be best

That sounds nice, let's do it this way.

You can just update the PR if you want :)

@windingwind
Copy link
Owner

btw, we also need an isChecked in that case, but only for type=check/radio menuitems. The type is immutable once the element is created and attached to the DOM.

@thebluepotato
Copy link
Author

If you're up to it, feel free to introduce isChecked

@thebluepotato
Copy link
Author

Also, should we rename onPopupShowing to onParentPopupShowing or something?

@windingwind
Copy link
Owner

Also, should we rename onPopupShowing to onParentPopupShowing or something?

Good point. Maybe onShowing would be enough?

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.

Menu manager optimizations
2 participants