feat(router-view): add router-view layouts feature #25

charlespockert · 2016-05-11T18:38:34Z

this allows a user to specify an additional view/viewmodel per route which is used as a "layout", this layout must contain a content selector where the content of the module specified in the route will be transcluded

charlespockert · 2016-05-11T18:47:57Z

Do we want unit tests - there are none at the moment for the router view?

EisenbergEffect · 2016-05-11T18:58:30Z

@charlespockert We'd love unit tests. We've got a new testing library that should make this easier as well.

EisenbergEffect · 2016-05-11T18:59:38Z

I haven't looked at the implementation yet...but this is really freaking awesome 👍

EisenbergEffect · 2016-05-11T19:10:26Z

Code looks pretty good. @charlespockert Would you be willing to help update with once the new shadow dom implementation is in place? Did you test it out with animation on?

@bryanrsmith @jedd-ahyoung Can you review?

alvarezmario · 2016-05-11T19:34:26Z

src/router-view.js

 export class RouterView {
  @bindable swapOrder;
+  @bindable layout;
+  @bindable layoutView;


@charlespockert I don't see this property used anywhere. A quick browser search shows it.

Also, I think instead of layout, we should use layoutView to specify the layout view instead and remove layout, that way should be more predictable for the user. Thoughts?

@alvarezmario Yep there are probably a couple of lines that need refactoring/removing as it was a back/forth job with a couple of contributors. There's also bug too which I've found/fixed on my local. Just wanted to get the code out there/some discussion going really.

@charlespockert Great. And what are your thoughts on using layoutView? compose has the same 3 properties, so users will be familiar with it.

@alvarezmario I actually took the conscious decision to use layout because I didn't like the caps in layoutView :)

We actually need layoutViewModel and layoutView (one for the module Id one for the view - neither are required or mutual). You can also use layoutModel too - on the element they would obviously be lower-case hyphenated.

So basically - I agree, it should be consistently named (I will tweak and PR again at some point this week)

charlespockert · 2016-05-11T20:14:18Z

@EisenbergEffect Absolutely happy to contribute (it's more fun than my day job!)

I've not checked it with animation but I'm thinking it should work since it's essentially the same pipeline and still enlists in the compositon/transaction and uses the same view slot - I'll try that now.

I'll also try and get some tests in place - I think it's wise to wait until the shadow DOM stuff is under way so we get an idea of what the API looks like for that, I'd like to be able to add named slots in here too which can pull parts of the target view in.

With this and viewports it probably gives you all the scenarios you'd want out the box (both sides of the same coin, so to speak)

charlespockert · 2016-05-12T13:28:16Z

https://gist.run/?id=677f5ca6a20b315a10d77d224be2cebb

Example of using named slots to reorganise the app layout in the template as per master pages/MVC sections.

This works fine with animation on my skeleton clone - just can't seem to get the animator kicking in on gist.run - must be missing something.

Edit: Ah - fixed it - updated the link above (forgot to link the stylesheet!)

I'll get some unit tests on there by the end of the week.

EisenbergEffect · 2016-05-16T12:19:26Z

Cool to see some tests!

I'm working on Shadow DOM v1 implementation now. It's about 75% done I think. Later this week, might be a good time for you to take a look at the templating branch so you can see how this would work with the new version.

I think this is a great feature. If you are up to it, there's another place this would also be useful: the compose element. Any interest in implementing this there as well once the Shadow DOM update is in place?

alvarezmario · 2016-05-16T14:01:01Z

@EisenbergEffect this is indeed a great featured. Previously I had to use child routers to achieve this, now it'll be easier. An example is homepage, which usually have a complete different layout than other pages in the application.
Are we gonna wait until RC to have this merged? I hope we don't.

charlespockert · 2016-05-16T17:22:03Z

@EisenbergEffect happy to take on whatever you need help with - give me a shout when you think the Shadow DOM stuff is complete enough to be pulled into this and I'll take a look.

EisenbergEffect · 2016-05-23T19:40:04Z

Blog post on the new Shadow DOM v1 implementation is here: http://blog.durandal.io/2016/05/23/aurelia-shadow-dom-v1-slots-prerelease/

this allows a user to specify an additional view/viewmodel per route which is used as a "layout", this layout must contain a content selector where the content of the module specified in the route will be transcluded. This allows developers to provide different layouts for different areas of their application similar to MVC layouts/ASP.NET master pages

charlespockert · 2016-05-24T21:30:42Z

@EisenbergEffect thought I'd give you a quick update. It's working with the Shadow DOM feature - I've not updated the package.json at the moment (since there's no npm release yet)

Tests run with the current config.js but that will probably need to change upon release (could do with some more tests as well).

Obviously usage is as you would expect:

layout.html

<template>
  <slot>fallback content</slot>
  <!-- or -->
  <slot name="slot1">hello</slot>
  <slot name="slot2">world</slot>
</template>

your-view.html

<template>
  <div>some content</div>
  <!-- or -->
  <div slot="slot1">some slot1 content</div>
  <div slot="slot2">some slot2 content</div>
</template>

EisenbergEffect · 2016-05-24T21:49:17Z

Cool. I think enabling this for the router is a real killer feature, especially since slots support fallback content.

For the moment, we can keep this PR open here. If you can, please continue to add more tests to any aspect of the code. That's a huge help.

Let me know if you have any other questions or concerns, otherwise, we'll probably merge this in in another week as we begin the preparations for RC.

EisenbergEffect · 2016-05-24T21:50:48Z

Just reviewed the code...looks like the new API we built made this cleaner to implement as well. How do you feel about it in the end? @charlespockert

alvarezmario · 2016-05-24T22:57:51Z

src/router-view.js

+    let viewSlot = new ViewSlot(layout.firstChild, false);
+    viewSlot.attached();
+
+    swapStrategy = this.swapOrder in swapStrategies


@charlespockert this code is repeated. Could you refactor it?

@alvarezmario yep certainly agree I'll refactor these

charlespockert · 2016-05-25T00:09:33Z

@EisenbergEffect yes the shadow DOM is a little cleaner content selectors on the whole but I did take a couple of small things out (no check to warn the developer that they forgot to put any slots in their template for example).

I'll iron out any potential niggles when I add more tests and refactor

charlespockert · 2016-05-26T19:31:07Z

@EisenbergEffect having some trouble here, seems like when I use the slot custom attribute in the tests that I'm trying to write, for some reason the DOM.Element key for DI is undefined so the container.get chokes

It works fine for non-named slots - any ideas?

I was thinking maybe mixed dependencies, but I've checked and I'm only loading 1 version of pal-browser/templating in the browser (the shadow DOM versions)

It's undefined here:

But not when the define() happens when the module is loaded...

EisenbergEffect · 2016-05-29T06:14:33Z

Are you tests set up to initialize the PAL? That's the only reason I can think that might cause that to be undefined.

charlespockert · 2016-05-29T10:00:44Z

@EisenbergEffect not explicitly, I'm initialising aurelia via the testing framework and it works fine with any tests that use the DOM without using the slot custom attribute.

I think there are other places where DOM.Element is injected fine, and if I debug I can see the PAL assign the DOM export on startup.

I'll keep digging, though I'm on holiday now until the weekend so progress will be minimal (and by that I mean zero)

EisenbergEffect · 2016-05-30T18:04:21Z

@charlespockert Do you have any interest in implementing this for the compose element as well?

charlespockert · 2016-05-30T18:51:19Z

Yes, I'll definitely have a go at that too in a few days when I'm back
On 30 May 2016 19:04, "Rob Eisenberg" notifications@github.com wrote:

@charlespockert https://github.com/charlespockert Do you have any
interest in implementing this for the compose element as well?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#25 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AIpzbl6WUwpP_rS0Us7Qb5UiNl_XU4FZks5qGyamgaJpZM4IcXcW
.

charlespockert · 2016-06-03T22:19:36Z

@EisenbergEffect ok, managed to get it all working, it was the call to initialize I was missing, the DOM object was there at the time of loading but Element was undefined at that point.

What's the plan for deps etc? If I just put this PR in as it is will you tweak deps as needed when you prep for release?

At the moment the deps for tests point to the github repos

alvarezmario · 2016-06-03T22:45:15Z

@charlespockert here aurelia/fetch-client@a97fa7b there are several library updates, so perhaps is better to have an individual PR for that.

EisenbergEffect · 2016-06-04T02:30:27Z

@charlespockert We'll fix up the deps. No need to worry about that.

charlespockert · 2016-06-07T13:50:15Z

@EisenbergEffect should be good to go I think - any suggestions from your side?

I've included deps needed in config.js to get the unit tests working but I've not updated the package.

EisenbergEffect · 2016-06-07T15:35:36Z

I'll do a final review in the next couple days and let you know if anything else is needed. Thanks!

AbraaoAlves · 2016-12-25T14:29:08Z

@charlespockert There any way to load module with html only like this layout feature ?

config.map([{route:'route', moduleId: 'test/module-view-only.html' }])

I'm searching for solution by days, but I did not find anything. So, I write a little modification in router-loader.js: commit and tests

This code is really necessary to load a html by route-view?

alvarezmario reviewed May 11, 2016
View reviewed changes

charlespockert added 2 commits May 24, 2016 22:12

chore(router-view): add routing/layout unit tests

71fac0a

alvarezmario reviewed May 24, 2016
View reviewed changes

alvarezmario mentioned this pull request May 31, 2016

replace-part of a parent view aurelia/templating#364

Closed

chore(router-view): refactor and update unit tests

4d5ae52

EisenbergEffect merged commit 4d5ae52 into aurelia:master Jun 8, 2016

AbraaoAlves mentioned this pull request Dec 23, 2016

Accept HTML only custom element in moduleId RouteConfig #49

Closed

smuszel mentioned this pull request Aug 11, 2018

View only routes importing incorrectly aurelia/loader-webpack#42

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(router-view): add router-view layouts feature #25

feat(router-view): add router-view layouts feature #25

charlespockert commented May 11, 2016

charlespockert commented May 11, 2016

EisenbergEffect commented May 11, 2016

EisenbergEffect commented May 11, 2016

EisenbergEffect commented May 11, 2016

alvarezmario May 11, 2016 •

edited

Loading

charlespockert May 11, 2016

alvarezmario May 11, 2016

charlespockert May 11, 2016 •

edited

Loading

charlespockert commented May 11, 2016 •

edited

Loading

charlespockert commented May 12, 2016 •

edited

Loading

EisenbergEffect commented May 16, 2016

alvarezmario commented May 16, 2016 •

edited

Loading

charlespockert commented May 16, 2016

EisenbergEffect commented May 23, 2016

charlespockert commented May 24, 2016

EisenbergEffect commented May 24, 2016

EisenbergEffect commented May 24, 2016 •

edited

Loading

alvarezmario May 24, 2016 •

edited

Loading

charlespockert May 25, 2016

charlespockert commented May 25, 2016

charlespockert commented May 26, 2016 •

edited

Loading

EisenbergEffect commented May 29, 2016

charlespockert commented May 29, 2016

EisenbergEffect commented May 30, 2016

charlespockert commented May 30, 2016

charlespockert commented Jun 3, 2016 •

edited

Loading

alvarezmario commented Jun 3, 2016

EisenbergEffect commented Jun 4, 2016

charlespockert commented Jun 7, 2016

EisenbergEffect commented Jun 7, 2016

AbraaoAlves commented Dec 25, 2016

feat(router-view): add router-view layouts feature #25

feat(router-view): add router-view layouts feature #25

Conversation

charlespockert commented May 11, 2016

charlespockert commented May 11, 2016

EisenbergEffect commented May 11, 2016

EisenbergEffect commented May 11, 2016

EisenbergEffect commented May 11, 2016

alvarezmario May 11, 2016 • edited Loading

Choose a reason for hiding this comment

charlespockert May 11, 2016

Choose a reason for hiding this comment

alvarezmario May 11, 2016

Choose a reason for hiding this comment

charlespockert May 11, 2016 • edited Loading

Choose a reason for hiding this comment

charlespockert commented May 11, 2016 • edited Loading

charlespockert commented May 12, 2016 • edited Loading

EisenbergEffect commented May 16, 2016

alvarezmario commented May 16, 2016 • edited Loading

charlespockert commented May 16, 2016

EisenbergEffect commented May 23, 2016

charlespockert commented May 24, 2016

EisenbergEffect commented May 24, 2016

EisenbergEffect commented May 24, 2016 • edited Loading

alvarezmario May 24, 2016 • edited Loading

Choose a reason for hiding this comment

charlespockert May 25, 2016

Choose a reason for hiding this comment

charlespockert commented May 25, 2016

charlespockert commented May 26, 2016 • edited Loading

EisenbergEffect commented May 29, 2016

charlespockert commented May 29, 2016

EisenbergEffect commented May 30, 2016

charlespockert commented May 30, 2016

charlespockert commented Jun 3, 2016 • edited Loading

alvarezmario commented Jun 3, 2016

EisenbergEffect commented Jun 4, 2016

charlespockert commented Jun 7, 2016

EisenbergEffect commented Jun 7, 2016

AbraaoAlves commented Dec 25, 2016

alvarezmario May 11, 2016 •

edited

Loading

charlespockert May 11, 2016 •

edited

Loading

charlespockert commented May 11, 2016 •

edited

Loading

charlespockert commented May 12, 2016 •

edited

Loading

alvarezmario commented May 16, 2016 •

edited

Loading

EisenbergEffect commented May 24, 2016 •

edited

Loading

alvarezmario May 24, 2016 •

edited

Loading

charlespockert commented May 26, 2016 •

edited

Loading

charlespockert commented Jun 3, 2016 •

edited

Loading