Skip to content
This repository has been archived by the owner on Sep 5, 2024. It is now read-only.

feat(list): add support for md-menu as proxied element #6459

Closed
wants to merge 1 commit into from
Closed

feat(list): add support for md-menu as proxied element #6459

wants to merge 1 commit into from

Conversation

devversion
Copy link
Member

@devversion devversion commented Dec 25, 2015 •
edited
Loading

  • Allow md-menu to be a proxied element in a list
  • Smart detection of being right or left aligned
  • Allows md-menu to be a secondary element
  • Adds accessibility support to the proxy trigger.
  • Support for RTL pages, automatically detecting the menu origin.

Preview (Secondary Item):

Fixes #3339

@devversion devversion force-pushed the feat/list-menu-support branch 3 times, most recently from 67ceec4 to f0fc577 Compare December 25, 2015 13:26
@devversion devversion mentioned this pull request Dec 25, 2015
Closed
devversion
devversion reviewed Feb 1, 2016
View reviewed changes
src/components/list/list.js
var buttonEl = menuEl.find('md-button') || menuEl.find('button');
if (!buttonEl || !buttonEl[0].parentNode == menuEl[0]) return;

if (!buttonEl[0].hasAttribute('ng-click')) buttonEl[0].setAttribute('ng-click', '$mdOpenMenu($event)');
Copy link
Member Author

Choose a reason for hiding this comment

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

This is not a good way? @EladBezalel

Copy link
Member

Choose a reason for hiding this comment

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

what do you mean by good way?

Copy link
Member Author

Choose a reason for hiding this comment

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

I mean, setting the ng-click attribute 😄

Copy link
Member

Choose a reason for hiding this comment

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

At first i would say you should use buttonEl.attr like the row below.
Secondly, this whole function feels odd to me.

Care to explain what your initial thought on this issue/PR

Copy link
Member Author

Choose a reason for hiding this comment

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

The code is old, I will change a few things.

The initial thought was, adding support for show a md-menu as a second element / second action / proxy element.

Copy link
Member Author

Choose a reason for hiding this comment

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

@EladBezalel - Updated the issue description + commit message + changed title + improved code.

@devversion devversion changed the title feat(list): add support for md-menu feat(list): add support for md-menu as proxied element Feb 1, 2016
@ThomasBurleson ThomasBurleson added needs: review This PR is waiting on review from the team in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs labels Feb 1, 2016
@ThomasBurleson ThomasBurleson added this to the 1.0.5 milestone Feb 1, 2016
EladBezalel
EladBezalel reviewed Feb 1, 2016
View reviewed changes
src/components/list/list.js
menuEl.attr('md-position-mode', (isLeftAligned ? 'target target' : 'target-right target'));

// Apply menu open binding to menu button
var menuOpenButton = menuEl.children().eq(0);
Copy link
Member

Choose a reason for hiding this comment

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

this is a weird line

Copy link
Member Author

Choose a reason for hiding this comment

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

This method is used by the original menu too (See here). I already had find, and validating the right parent. I think using the first children is the right way to get the triggering element.

@devversion devversion force-pushed the feat/list-menu-support branch 2 times, most recently from 666308c to 6ab2e2b Compare February 2, 2016 12:54
@ThomasBurleson ThomasBurleson modified the milestones: 1.0.5, 1.0.6 Feb 4, 2016
@devversion devversion mentioned this pull request Feb 13, 2016
Closed
@devversion devversion added needs: rebase This PR needs to be rebased on the latest commits from master and conflicts need to be resolved and removed in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs labels Mar 21, 2016
@ThomasBurleson ThomasBurleson modified the milestones: 1.0.6, 1.0.8 Apr 4, 2016
@ThomasBurleson
Copy link
Contributor

@devversion - please rebase, then LGTM for 1.1.0.

@ThomasBurleson ThomasBurleson removed the needs: review This PR is waiting on review from the team label Apr 19, 2016
@ThomasBurleson ThomasBurleson added this to the 1.1.1 milestone Apr 19, 2016
@ThomasBurleson ThomasBurleson removed this from the 1.0.8 milestone Apr 19, 2016
@ThomasBurleson ThomasBurleson added the in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs label Apr 19, 2016
@ThomasBurleson ThomasBurleson modified the milestones: 1.1.1, Backlog Apr 21, 2016
@ThomasBurleson ThomasBurleson modified the milestones: - Backlog, Deprecated May 26, 2016
@devversion devversion modified the milestones: - Backlog, Deprecated May 27, 2016
@devversion
feat(list): add support for md-menu as proxied element
c7fd83c 
* Allow md-menu to be a proxied element in a list
* Smart detection of being right or left aligned
* Allows md-menu to be a secondary element

* Support for RTL pages, automatically detecting the menu origin.

Fixes angular#3339
@devversion devversion added needs: review This PR is waiting on review from the team and removed in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs needs: rebase This PR needs to be rebased on the latest commits from master and conflicts need to be resolved labels May 27, 2016
@devversion
Copy link
Member Author

@ThomasBurleson This is now rebased and updated.

  • Also added better RTL support for the menu proxy.

@devversion devversion deleted the feat/list-menu-support branch June 11, 2016 15:56
@devversion devversion mentioned this pull request Aug 26, 2016
Closed
@Splaktar Splaktar removed this from the - Backlog milestone Jun 28, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@ThomasBurleson ThomasBurleson

Labels
needs: review This PR is waiting on review from the team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@devversion @ThomasBurleson @EladBezalel @rschmukler @Splaktar