Add support for params in menu items #1305
Conversation
| @@ -378,6 +379,10 @@ | |||
| it('should set position to options position', function() { | |||
| expect(subItem1.position).toEqual(subItemOptions.position); | |||
| }); | |||
|
|
|||
| it('should set params to options params', function() { | |||
| expect(subItem1.params).toEqual(subItemOptions.params); | |||
There was a problem hiding this comment.
There's a trailing space here causing the build to fail.
|
Sorry. I added that to force another commit to put in a better commit message, then realized you get a second chance on the pull request. Resolved now. |
| @@ -85,6 +85,7 @@ | |||
| service.menus[menuId].items[itemIndex].items.push({ | |||
| title: options.title || '', | |||
| state: options.state || '', | |||
| params: options.params || '', | |||
There was a problem hiding this comment.
Set this to an empty object, for consistency.
|
Overall, this looks good to me. I think this will be a great enhancement. Would it be possible to add an E2E test for this as well? We don't have E2E coverage for the core module at this time. I'm not sure how complicated it will be to add E2E tests for the menu service, but it would be great to have. |
|
I agree more E2E tests would be a good thing. I upgraded from an earlier mean.js stack and was happy to see they were incorporated now. But it is still on my TODO list to learn how to use them effectively, and I am definitely not the best to lay down precedence for how to test core E2E. I'm not even sure if E2E tests fit well for core type functionality, since it is more building block than actual UI in most cases. It might make more sense to simply add some different use cases to a module like articles that exercises the core functionality and run the E2E tests there. Someone with more experience with E2E might have more insight there. |
|
I agree with the "core type functionality" not really fitting in well with E2E. However, the Menu service is a UI feature. I think we can just roll with what you have here. I'll wait for others to provide feedback. On a side note: One other thing that might be confusing to our users is that the Menu's default to having roles of |
|
LGTM, but I would like to see some more tests if possible. |
|
@mleanos I concur the menu service could stand for an overhaul / upgrade. It feels like the weakest part of the stack from the parts I've been working with anyway. It is a lot better than previous versions which were barely documented, but still not all that easy to work with. I've ended up only using it only for the high level navigation and building my own for a left navigation style based on Material elements. I think trying to leverage Material in a standard way would be the way to go now that Material is included in the mean.js stack. |
|
@scfox I think your proposal for the params is great but I don't see material included in mean? Did I miss something? Is it workable combining material with bootstrap in mean? |
|
@trendzetter My mistake, Material is not included in the current mean stack, I added it in myself. It is workable to combine though. Here is a tutorial on how to do it: http://www.bossable.com/1231/material-design-meanjs/ |
|
What's the status of this PR? |
|
I think it is ready to be pulled/merged. No other feedback came back on it. |
|
I just reviewed this again. LGTM. |
|
@mleanos , @scfox , can you also add a small documentation on how to use it? you can either do that by doing a Pull Request to our documentation gh-pages repo/branch ( @codydaig can help you with that ), or just put a snippet of code example here and one of us will take care of updating the docs properly. Other than that, if LGTY then let's merge please. |
|
@scfox Would you be able to create a PR that adds some documentation on how the parameters work? And perhaps an example of how to integrate it into a client-side route/state? I wasn't able to install Jekyll on Windows when I last tried, so it's difficult for me to test locally. I'd be more than happy to review though. |
|
Merging, since this PR adds a test for the new functionality. The documentation can wait for a new PR. @scfox Thank you for this contribution. Would you be interested in adding a PR for an update to our documentation on the Menus Service? |
feat(core): Add support for params in menu items
Proposed by @scfox
Fixes #1304