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

Refactors SC.MenuPane to use SC.ListView internally #1443

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

Conversation

nicolasbadia
Copy link
Member

SC.MenuPane no use SC.ListView internally in order to be able to display huge lists of items.

  • Events are now dispatched to the list view.
  • SC.MenuItemView now extend SC.ListItemView and uses SC.Control instead of SC.ContentDisplay.
  • Selection is now managed with SC.SelectionSupport.
  • This commit also add a new actOnSubMenu property, and allows separators to have a title.
  • SC.MenuSeachPane height is also now correctly computed, which solve an issue when only one item is displayed.
  • It removes support for itemExampleViewKey as it is complicated to implement with SC.ListView and doesn't really make sense.
  • It also removes localization support as this is not supported by SC.ListView.
  • I also included a CSS cleanup and removed SC.ContentDisplay as it is not used anymore

@nicolasbadia nicolasbadia force-pushed the team/nicolasbadia/menu-refactor branch 2 times, most recently from a9fe6e6 to afe8c84 Compare February 22, 2018 09:56
@mauritslamers
Copy link
Member

I disagree with removing SC.ContentDisplay. It might not be used here, but it is very useful to have in many other circumstances.

@nicolasbadia nicolasbadia force-pushed the team/nicolasbadia/menu-refactor branch 2 times, most recently from 33b7d91 to 7630c79 Compare February 25, 2018 17:07
@nicolasbadia
Copy link
Member Author

I reverted the SC.ContentDisplay removal and added support for itemUnreadCountKey.
I also fixed an issue with SC.Record and added a test for it.

@nicolasbadia nicolasbadia force-pushed the team/nicolasbadia/menu-refactor branch 2 times, most recently from 32125eb to e6bbb5e Compare February 26, 2018 23:40
@nicolasbadia
Copy link
Member Author

I updated the last commit with the suggested adjustements.

@mauritslamers
Copy link
Member

A few things:

The commit 7f7e730 turns out to be responsible for the failing tests (https://travis-ci.org/sproutcore/sproutcore/builds/350716397#L1740), as a change in the title of an item in the items is no longer picked up as there is no mechanism for it.

Also I think the commit history is a bit messy, as it removes and then re-adds functionality. It also includes changes to for example the designer framework, which is more or less dead weight IIRC.
I would therefore suggest that we break this PR up in a few separate ones and also squash a few commits in order to get a more logical commit history.

There seems to be a contradiction in the description up, specifically where you mention that you remove the localization support but in the resulting code the localize setting on the view is still there. Is the change that you removed the option to automatically localize all the menu items?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants