Skip to content
This repository has been archived by the owner on Feb 2, 2019. It is now read-only.

md-menu first implementation #200

Closed
wants to merge 3 commits into from

Conversation

aminebizid
Copy link
Contributor

@aminebizid aminebizid commented Apr 26, 2016

This is a very basic implementation of md-menu.
Keep tuned :)

</md-menu-item>
</md-menu-content>
</md-menu>
</div>
Copy link
Collaborator

Choose a reason for hiding this comment

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

blank line at the end of the file is missing

@Gregcop1
Copy link
Collaborator

First of all, thanks for you contribution. You may think that I've made a lot of comments but I think, you've used a lot of direct DOM manipulation instead of making use of Angular cool stuff, like templates, children reference, query, ...
May be you should write some tests during development. Sometimes it helps to see that if it's complicated to test, it's because the code is too complex.
Keep going! It's a "must have" feature :)

@aminebizid
Copy link
Contributor Author

I didn't find how to append a child in Angular without using the DOM.

@Gregcop1
Copy link
Collaborator

Gregcop1 commented Apr 26, 2016

To append a child to the body, you're right. MdDialog do the same thing, but IMO your container should be a component itself and all other dom manipulation (style, attribute, ...) should be made via templates and variable in this new component

"rules": {
"class-name": true,
"comment-format": [true, "check-space"],
"curly": true,
Copy link
Collaborator

@Gregcop1 Gregcop1 Apr 26, 2016

Choose a reason for hiding this comment

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

why changing tslint rules ? Current code is based on those rules

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry it's a mistake to commit tslint.json

Envoyé de mon iPhone

Le 26 avr. 2016 à 18:10, Grégory Copin notifications@github.com a écrit :

In tslint.json:

"rules": {

  • "class-name": true,
  • "comment-format": [true, "check-space"],
  • "curly": true,
    why changing tsling rules ? Current code is based on those rules


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub

@aminebizid
Copy link
Contributor Author

Ok I'll create a child template. I was avoiding that to make the code compact 1 component 1 template for basic menu.
This will make nested menus more complex.

@Gregcop1
Copy link
Collaborator

Gregcop1 commented Apr 26, 2016

If it's an intern component, it's not more complex for the user, so it doesn't matter. But with a container append to body with overlay, one day, we can think of an unified component for all overlay (dialog, etc.). The more you rely on intern code, the more specific you are. May be we can think one step further.

@aminebizid
Copy link
Contributor Author

Ok let's create a share container component for all popups and we put it in the body.

@justindujardin
Copy link
Owner

@Gregcop1 @Zigzag95 cool work! Before you go creating an overlay system, please consider using the @material2/core npm package. It includes an overlay system for exactly this purpose.

I plan to replace components with the official material2 implementations as they're available, so there's no conflict bringing in the core package now.

Please examine their portals and overlay core classes:
https://github.com/angular/material2/tree/master/src/core/portal
https://github.com/angular/material2/tree/master/src/core/overlay

The positioning strategy you will want to use for a menu is currently being reviewed here: angular/components#335

@Gregcop1
Copy link
Collaborator

So how do we procede? Should we include dependency to official overlay system ?

@justindujardin
Copy link
Owner

@Gregcop1 yes, let's do. I haven't used it yet so I can't recommend a way, but please investigate their demos and take a shot at it.

Please create a branch on the main repo to work on this. I have added @Zigzag95 as a collaborator so both of you may work on it together to make things easier.

Once you guys have a branch to work from, please close this PR and create a new one from that branch.

@zAPFy
Copy link
Collaborator

zAPFy commented Apr 27, 2016

I have started some work on an autocomplete component. #202
I guess I could also use the Overlay class for that.
I will put the dev of autocomplete on hold as to not duplicate the effort.

@aminebizid
Copy link
Contributor Author

WIP on overlay. It may take some days.
@core integration is not so trivial.

Envoyé de mon iPhone

Le 27 avr. 2016 à 18:08, Alexander Zeiher notifications@github.com a écrit :

I have started some work on an autocomplete component. #202
I guess I could also use the Overlay class for that.
I will put the dev of autocomplete on hold as to not duplicate the effort.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

@zAPFy zAPFy mentioned this pull request May 6, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants