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

fix(md-menu): allow nested md-menu-content again #11103

Merged
merged 1 commit into from
Feb 21, 2018
Merged

fix(md-menu): allow nested md-menu-content again #11103

merged 1 commit into from
Feb 21, 2018

Conversation

fsmeier
Copy link
Contributor

@fsmeier fsmeier commented Feb 1, 2018 •
edited
Loading

PR Checklist

Please check that your PR fulfills the following requirements:

  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?
Fixes misbehavior from #10198

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: #10540

Does this PR introduce a breaking change?

[ ] Yes
[x] No

@googlebot googlebot added the cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ label Feb 1, 2018
@fsmeier
Copy link
Contributor Author

fsmeier commented Feb 1, 2018

@Splaktar Somehow the test seems to fail because of a different reason than my code. Do you know what happened here?

devversion
devversion approved these changes Feb 2, 2018
View reviewed changes
Copy link
Member

@devversion devversion left a comment

Choose a reason for hiding this comment

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

LGTM. One minor comment

src/components/menu/js/menuDirective.js Outdated
if (!contentEl || contentEl.nodeName !== 'MD-MENU-CONTENT') {
throw Error(INVALID_PREFIX + 'Expected the menu to contain a `md-menu-content` element.');
if (templateElement.children().length !== 2) {
throw Error(INVALID_PREFIX + 'Expected two children elements.');
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to still kind of refer to the md-menu-content element.

@devversion devversion added the pr: lgtm This PR has been approved by the reviewer label Feb 2, 2018
@fsmeier
Copy link
Contributor Author

fsmeier commented Feb 5, 2018

@Splaktar can this be added to milestone/release 1.1.7 or is it too late?

@Splaktar Splaktar added this to the 1.1.8 milestone Feb 11, 2018
@Splaktar Splaktar added severity: regression This issue is related to a regression P1: urgent Urgent issues that should be addressed in the next minor or patch release. type: bug needs: squash commits in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs labels Feb 11, 2018
@Splaktar
Copy link
Contributor

@IMM0rtalis there wasn't time to get this into 1.1.7 due to how long of a process it is to get through presubmits. Please squash your commits and then I'll send this off for presubmit testing.

@fsmeier fsmeier force-pushed the fix/allow-nested-md-menu-content branch from bcfe1de to 0c09b8f Compare February 15, 2018 14:17
@fsmeier
Copy link
Contributor Author

fsmeier commented Feb 15, 2018

@Splaktar done.
Yeah, I thought so when I saw 1.1.7 released ;)

@Splaktar Splaktar added pr: merge ready This PR is ready for a caretaker to review and removed needs: squash commits in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs labels Feb 18, 2018
@josephperrott josephperrott merged commit 60e2393 into angular:master Feb 21, 2018
@fsmeier fsmeier deleted the fix/allow-nested-md-menu-content branch February 22, 2018 07:55
@Splaktar
Copy link
Contributor

@IMM0rtalis in order to improve the generated changelog and make sure that the related issue gets closed, please include Fixes #10540 on the last line of your commit message in the future. More details are in the commit message guidelines.

chmelevskij pushed a commit to chmelevskij/material that referenced this pull request Jun 19, 2018
@chmelevskij
fix(md-menu): allow nested md-menu-content (angular#11103)
8f76977
Splaktar pushed a commit that referenced this pull request Jul 31, 2018
@Splaktar
fix(md-menu): allow nested md-menu-content (#11103)
3e57ee9
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
1 more reviewer

@devversion devversion devversion approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@Splaktar Splaktar

Labels
cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ P1: urgent Urgent issues that should be addressed in the next minor or patch release. pr: lgtm This PR has been approved by the reviewer pr: merge ready This PR is ready for a caretaker to review severity: regression This issue is related to a regression type: bug
Projects
None yet
Milestone
1.1.8
Development

Successfully merging this pull request may close these issues.
5 participants
@fsmeier @Splaktar @devversion @googlebot @josephperrott