QwikDev / qwik-evolution

Home for Qwik proposals and RFCs
15 stars 0 forks source link

In layout, multiple slots with name attributes cannot be supported[✨] #103

Closed miguba closed 1 month ago

miguba commented 1 year ago

Is your feature request related to a problem?

In layout, multiple slots with name attributes cannot be supported.

Describe the solution you'd like

Can multiple slots be supported in the layout?

Describe alternatives you've considered

no idear

Additional context

No response

manucorporat commented 1 year ago

How would this feature look like? i cant see how named slots are useful in layouts? how pages would specify the named slot?

dhdemerson commented 1 year ago

I found myself wanting this for a layout that consisted of a sidebar and content area. On child routes I wanted to add sub-navigation specific to the child route to the sidebar while rendering the main child content into the content area.

Without something like named slots in layout, would the recommended pattern be to keep the logic in the parent layout to conditionally render the appropriate sub-navigation based on the active child route?

I'm just learning Qwik so I won't speculate on a possible API (or if it's even a good idea to introduce named layout slots).

Possibly related: https://github.com/BuilderIO/qwik/issues/2429

genki commented 1 year ago

I think useContent() can be used for that purpose, but it would be nice if it's possible to use a component instead of the menu.md. For ex, expected the <Slot name={what} /> in the layout is projected from ./<what>.tsx (if it exists) as like as the <Slot /> is projected from ./index.tsx

julianobrasil commented 1 year ago

@adamdbradley, @manucorporat I like @genki's suggestion. If we want to make it clear that the content is to be projected to a named slot, some token could be introduced in the name, like we do when we want to use a specific layout (by following the index@layout-name.tsx pattern).

Maybe for the slots we could use something in that direction, for example using squared brackets:

Pterygoidien commented 1 year ago

Hi, same thing : it would be great ! I tried to make a side navbar for custom routes on the left for an educational platform, where the menu would vary depending on the part of the tree of the course (section/chapters/pages), and thought named Slots would be ideal in layout, where we could populate the link in the layout through the Slot identifier. The useContent() needs a lot of workarounds to come to that result (didn't succeed yet).

dgknca commented 1 year ago

this is essential. still waiting.

mhevery commented 1 year ago

How do other meta-fws solve this problem? Can you point to examples?

dgknca commented 1 year ago

How do other meta-fws solve this problem? Can you point to examples?

I wanted to have something like this:

// layout.tsx
export default component$(() => {
  return (
    <>
      <div class='py-5'>
        <Slot name='header' />
        <section>
          <Slot />
        </section>
      </div>
    </>
  );
});
// index.tsx
export default component$(() => {
  return (
    <>
      <div q:slot='header'>header items</div>
      <div>
         article content
      </div>
    </>
  );
});

named slots are not working in layout.

mhevery commented 1 year ago

So the issue is show here: https://stackblitz.com/edit/qwik-starter-1pnxsm?file=src%2Froutes%2Findex.tsx

import { Slot, component$ } from '@builder.io/qwik';

export default component$(() => {
  return (
    <Layout>
      <Index />
    </Layout>
  );
});

export const Layout = component$(() => {
  return (
    <>
      <div class="py-5">
        <Slot name="header" />
        <section style={{ border: '1px solid red' }}>
          <Slot />
        </section>
      </div>
    </>
  );
});
// index.tsx
export const Index = component$(() => {
  return (
    <>
      <div q:slot="header">header items</div>
      <div>article content</div>
    </>
  );
});

The basic problem is that the <Index/> gets plated into <Layout/> like this:

<Layout>
  <Index/>
</Layout>

So the q:slot is to low in the hierarchy. If you want them projected it would have to be something like this:

<Layout>
   <div q:slot="header">header items</div>
  <Index/>
</Layout>

And that is not how routes are composed.

I think this would be an issue for other meta-frameworks as well. How do they solve this problem? (I can't think of a way they would solve this)

genki commented 1 year ago

@mhevery
I came up with a workaround that should work, but it doesn't. https://stackblitz.com/edit/qwik-starter-6udjqi?file=src%2Froutes%2Findex.tsx

If we use the useVisibleTask$ instead of the useTask$ then it works. https://stackblitz.com/edit/qwik-starter-nxhuhk?file=src%2Froutes%2Findex.tsx

Might found a bug.

mhevery commented 1 year ago

@genki

I came up with a workaround that should work, but it doesn't. https://stackblitz.com/edit/qwik-starter-6udjqi?file=src%2Froutes%2Findex.tsx

Right, this is because the <Header> gets rendered first and is streamed into the HTTP response, and then you change the state of the system after the <Header> has already been sent to the browser. So this is working as intended, but it would be nice to get a warning or something to let you know that you have changed the signal after it was used to render output.

The useVisibleTask$ works because you do the rendering on the client, but I would like to discourage you from using that as you are eagerly executing code on the client.

PS: Sorry, I don't have a good answer for you.

genki commented 1 year ago

@mhevery Oh, I see. And I fixed the workaround to work fine this time. https://stackblitz.com/edit/qwik-starter-6udjqi?file=src%2Froutes%2Findex.tsx As you explained, this trick can be used only for footers and so on :)

genki commented 1 year ago

@mhevery Finally, I solved this issue. How about do this? https://stackblitz.com/edit/qwik-starter-l8gnoe?file=src%2Froutes%2Findex.tsx

mhevery commented 1 year ago

Great! Can this issue be closed?

remypar5 commented 1 year ago

@mhevery I don't believe it can be closed. What is described here is a workaround. Although it does work, it doesn't look very good and requires some scaffolding per layout and page.

Also a first-time user of Qwik. Loving it so far. I would suggest something that is similar that works out of the box:

// layout.tsx
export default component$(() => {
  return (
    <div class="wrapper">
      <Header />
      <Menu />
      <Slot name="submenu" />
      <Slot />
      <Slot name="sidebar" />
      <Footer />
    </div>
  )
})
// index.tsx
// Applied to the default Slot
export default component$(() => (<p>page content</p>))

// Applied to the named slots, if available
export const layoutSlots = {
  sidebar: () => (<p>sidebar content</p>)),
  submenu: () => (<p>submenu content</p>)),
}

Should a layout's named slot not be defined in the page, it'll just remain empty. If a page defines a named Slot which is not available in the layout, it'll be ignored. In short: "If both are available, assign it to the slot. If either is unavailable, ignore it"

This way we can keep it clean and consistent.

mhevery commented 1 year ago

OK, I like your proposal and agree that this would be a good feature. (Sorry it took me so long to understand what you are asking.)

Any chance you would be up for creating a PR for this? I would be happy to guide you through it.

remypar5 commented 1 year ago

@mhevery Yes, I would love to do that. And get some pointers on where to go. After reading CONTRIBUTING.MD, of course.

mhevery commented 1 year ago
Okm165 commented 1 year ago

How is this going forward? Kind of stopping my development for it, no use in finding dirty workarounds

oqx commented 1 year ago

Just chiming in to +1. The benefit of layout is limited if I'm having to create a supplemental layout component to place inside of each route anyway for multi-slot support. Spreading layout across multiple files is kind of a PITA.

remypar5 commented 1 year ago

Hi, I started to work on this, but I don't seem to have the time anymore. I got to the point where caching the layoutSlots worked fine. That was the easy part. The hard part was to go further up the tree to find a Slot q:name="..." component to assign any cached elements to. Last week I started a new job which consumes my full bandwidth.

wmertens commented 10 months ago

@mhevery wouldn't it be possible to support this by skipping fragments when resolving slots? Then the <Index /> situation you explain in https://github.com/BuilderIO/qwik/issues/2442#issuecomment-1687240269 would just work?

Maybe you can address this in the v2 branch?

pustovitDmytro commented 10 months ago

Would be great to have the ability of dynamic layouts, like in DocumentHead:

export const layoutSlots = ({ resolveValue }) => {
    const receipt = resolveValue(useRecipesDetails);

    return {
        headerContent : receipt
            ? <HeaderContent receipt={receipt}></HeaderContent>
            : null
    };
};

export const head: DocumentHead = ({ resolveValue }) => {
    const receipt = resolveValue(useRecipesDetails);

    return {
        title : receipt ? receipt.title : 'Tastoria Receipt'
    };
};
wmertens commented 10 months ago

Actually just looking up the tree might be problematic. How about q:slot="../title" so you specify exactly which parent? That way it's probably also easier to implement.

gioboa commented 1 month ago

We moved this issue to qwik-evolution repo to create a RFC discussion for this. Here is our Qwik RFC process thanks.