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

Conversation

devversion
Copy link
Member

  • This moves the contentElement logic from the $mdDialog (as introduced in 135cb3a) to the $mdCompiler.

    This allows the $mdPanel to take advantage of this functionality and does not block the port of the dialog to the $mdPanel

FYI: Don't be too nitpicky about the docs, those were mostly not written by me (rather fix those in a new PR)

Base for #9366

@devversion devversion added the needs: review This PR is waiting on review from the team label Sep 8, 2016
@devversion devversion added this to the 1.2.0 milestone Sep 8, 2016
@devversion devversion added the needs: rebase This PR needs to be rebased on the latest commits from master and conflicts need to be resolved label Sep 10, 2016
@ThomasBurleson
Copy link
Contributor

@topherfangio, @ErinCoughlan - plz review.

@ThomasBurleson ThomasBurleson modified the milestones: 1.1.2, 1.2.0 Sep 10, 2016
@devversion devversion force-pushed the feat/compiler-content-element branch from eaf4a6d to 272e1be Compare September 10, 2016 11:46
@devversion devversion removed the needs: rebase This PR needs to be rebased on the latest commits from master and conflicts need to be resolved label Sep 10, 2016
@topherfangio
Copy link
Contributor

LGTM. Definitely agree that the docs need some cleanup, but otherwise, I think this is good.

* to easily compile an element with options like in a Directive Definition Object.
*
* > The compiler powers a lot of components inside of Angular Material.
* > Like the `$mdPanel` or `$mdDialog`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Hehe, this line is great. 💃

I'm fine with fixing the docs later, but some love would be nice.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hehe, I only have love available for the $$interimElement stuff 😄

As planned, we are improving this in a follow-up PR

@devversion devversion force-pushed the feat/compiler-content-element branch from 272e1be to 9154af4 Compare September 12, 2016 18:42
@devversion
Copy link
Member Author

@topherfangio Yeah, as said I did not write most of the docs, and I wanted to keep this PR as clean as possible, since changes to the compiler are very dangerous.

There will be a follow-up PR for improving the documentation ;)

*/
function MdCompilerService($q, $templateRequest, $injector, $compile, $controller) {
/** @private @const {!angular.$q} */
this.$q = $q;
Copy link
Member

Choose a reason for hiding this comment

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

Probably down to preference, but if these are private, shouldn't they be prefixed with an underscore?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, I do not like to have them in the Prototype, we also don't have them in the chips.

I personally like them, when working with TypeScript and Getters (as a accumulator)

@devversion devversion force-pushed the feat/compiler-content-element branch from 9154af4 to 42071c0 Compare September 12, 2016 19:08
@crisbeto
Copy link
Member

A couple of very minor notes, but otherwise LGTM.

Copy link
Contributor

@ErinCoughlan ErinCoughlan left a comment

Choose a reason for hiding this comment

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

LGTM

* @returns {{element: !JQLite, restore: !Function}}
* @private
*/
MdCompilerService.prototype.fetchContentElement = function(options) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Private methods should have underscores in the name.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I missed that one. Solved now!

@devversion devversion force-pushed the feat/compiler-content-element branch from 42071c0 to 324d179 Compare September 15, 2016 16:42
Copy link
Contributor

@ThomasBurleson ThomasBurleson left a comment

Choose a reason for hiding this comment

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

LGTM

@ThomasBurleson ThomasBurleson removed the needs: review This PR is waiting on review from the team label Sep 15, 2016
@jelbourn jelbourn merged commit dfe1a00 into angular:master Sep 21, 2016
@devversion devversion deleted the feat/compiler-content-element branch September 21, 2016 15:29
Frank3K pushed a commit to Frank3K/material that referenced this pull request Oct 8, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants