AgnosticUI / agnosticui

AgnosticUI is a set of UI primitives that start their lives in clean HTML and CSS. These standards compliant components are then copied to our framework implementations in: React, Vue 3, Angular, and Svelte.
https://agnosticui.com
Apache License 2.0
723 stars 47 forks source link

Menu for Angular components library #178

Closed vitale232 closed 2 years ago

vitale232 commented 2 years ago

Pull Request Template

Description

Implements AgnosticUI Menu for Angular Component Library, accepting content for the menu items.

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context.

Fixes # (issue)

Checklist:

Please delete options that are not relevant.

- [ ] Is this a bug fix - [ ] Does the change require CSS updates to existing components? If so, have you updated all frameworks and verified?

We realize above checklist is pretty intimidating, reach out with an at mention on your issue if you're interested in contributing but need some clarifications!

vitale232 commented 2 years ago

@roblevintennis I took a couple of swings at cleaning up the commit log with an interactive rebase, but I kept getting locked up. Let me know if you would like that to be done and I can work through it (not something I have much experience with).

Biggest change from the last time you reviewed this is on the Angular Dependency Injection side. I previously was trying to use the MenuComponent as a type in ng's DI. That leads to a circular dependency. ng-packagr would still build it, but Storybook wants nothing to do with it. So I've abstracted the MenuComponent to an interface, which the component then implements. It'll be a little bit trickier to maintain, but since the component implements the interface, there will be errors

roblevintennis commented 2 years ago

Thanks for the updatesπŸ™πŸ½

Don't worry about the commit log for now but thanks for trying.

I had a pretty quick look through and nothing stood out egregious so that's good πŸ‘πŸ½

It's a pretty big PR so I think it's a good idea for me to fetch your remote and do a round of spot checks. My weekend is is shot with sick kiddos to watch so I'll try to get to it early next week. Thanks again!

roblevintennis commented 2 years ago

@vitale232 Thank you so much for your efforts on this tricky component! I went ahead and did a pretty thorough QA and created https://github.com/AgnosticUI/agnosticui/issues/181 for follow up fixes. I think this is a significant step forward and worth it to just merge it for now. There's another reason to…

I'm midway on a fairly major refactor and I'd like to get your PR here merged so I can do the minor rewiring of some of the style copies from copystyles.js. I've left details on the follow up issue but basically I started to break out some of the buttons.css in ways that will sort of break all framework implementations πŸ˜‹ if I don't do this rewiring. It's an interesting problem and perhaps drawback of AgnosticUI's shared CSS approach that I'll have to contemplate but I think it's fine for now.

Let me know if you feel compelled to take on https://github.com/AgnosticUI/agnosticui/issues/181 or some of the items (I can break that up into separate issues if it helps). Well done here though! I'm so happy you contributed this nice improvement to the Angular package! πŸ’― πŸ”₯ πŸ™Œ 🐨 πŸ˜‰

vitale232 commented 2 years ago

@vitale232 Thank you so much for your efforts on this tricky component! I went ahead and did a pretty thorough QA and created #181 for follow up fixes. I think this is a significant step forward and worth it to just merge it for now. There's another reason to…

I'm midway on a fairly major refactor and I'd like to get your PR here merged so I can do the minor rewiring of some of the style copies from copystyles.js. I've left details on the follow up issue but basically I started to break out some of the buttons.css in ways that will sort of break all framework implementations πŸ˜‹ if I don't do this rewiring. It's an interesting problem and perhaps drawback of AgnosticUI's shared CSS approach that I'll have to contemplate but I think it's fine for now.

Let me know if you feel compelled to take on #181 or some of the items (I can break that up into separate issues if it helps). Well done here though! I'm so happy you contributed this nice improvement to the Angular package! πŸ’― πŸ”₯ πŸ™Œ 🐨 πŸ˜‰

Thanks so much for the thorough review! Glad we were able to move the libs forward a bit.

I'm definitely interested in helping out with #181. I'll probably only have a few hours per week to dedicate to this, so as long as that's not a problem I'm on board!

I've also been thinking about adding some structure into the ng lib. Specifically, I think it would be nice to start splitting the components into their own little modules. ag-ui is pretty well positioned to be one of the first Angular libs to embrace standalone components, assuming that lands soon. So, perhaps instead of having a kitchen sink AgModule, we would have AgMenuModule. Then, when Angular lands standalong components, we'd be positioned to migrate to the public API becoming imports: [AgButtonComponent, AgMenuComponent] in the consumer's metadata. If that's something you're interested in, too, I can give it a bit more thought an open an issue.

roblevintennis commented 2 years ago

Wow that RFC is huge lol. I know that I got tripped up on NgModule when I started my job (not quite a year ago) where we use Angular. Interesting that they're looking into these "standalone components" which is really much closer to other framework ecosystems. I have no idea which is or will be "better" in the Angular landscape. Honestly, I'd assume you have a better sense of that pulse. I'd definitely be open to the approach πŸ‘

ag-ui is pretty well positioned to be one of the first Angular libs to embrace https://github.com/angular/angular/discussions/43784

Maybe you're trying to say this could be a bit of a differentiator? Ok, cool 😎 πŸ‘

In terms of helping with #181 a few hours per week is wonderful and appreciated :)

I don't know if I mentioned but I'm really hoping folks such as yourself with more Angular experience can help to shape the Angular package. I did it primarily because 1. Angular is still very popular and used in a lot of important apps so I wanted to support it 2) It helps me to slowly get better at Angular myself which in turn helps me at work.

But I'm still not very good at it so it really helps to have someone come in and create a directive, mention upcoming RFCs and interesting directions! πŸ™