reflex-dev / reflex

🕸️ Web apps in pure Python 🐍
https://reflex.dev
Apache License 2.0
18.21k stars 1.01k forks source link

about use rx.memo with children #3279

Open seewindcn opened 2 months ago

seewindcn commented 2 months ago

Describe the bug I am trying use memo for website's layout, like this:

@rx.memo
def layout1(children: rx.Component) -> rx.Component:
  from antd_demo.components import footer, navbar, header, subnav, content
  return layout.layout(
        header(),
        layout.layout(
            navbar(),
            layout.layout(
                subnav(),
                content(
                    children,
                )
            )
       )

it work; but the js like below:

<Layout1
            children={[xxxxxxxxxxxxxxxxxxx............]}>
            <RadixThemesFlex css={{"display": "flex"}}>
                <RadixThemesFlex align={`start`} css={{"height": "81vh", "width": "100%"}} >
                    <Reactmarkdown_6c4d2df3472374f2de5b35860d0c49c3/>
                    <RadixThemesFlex css={{"flex": 2, "justifySelf": "stretch", "alignSelf": "stretch"}}/>
                </RadixThemesFlex>
            </RadixThemesFlex>
        </Layout1>

my question is:

  1. why layout1 has children property? it is very long and useless. children is a special property in jsx.
  2. not doc for memo's usage;
  3. style of memo component isn't support;

Specifics (please complete the following information):

Additional context Add any other context about the problem here.

picklelo commented 2 months ago

@seewindcn The memo component was a bit hacky and needs to be documented better for sure.

For your use case, the following code works for me:

import reflex as rx

@rx.memo
def layout1(children: rx.Component) -> rx.Component:
  return rx.container(
        rx.heading("heading"),
        children,
    )

def index():
    """The main view."""
    return layout1(
        rx.box("hi")
    )

app = rx.App()
app.add_page(index)

How are you passing in the children? Since it's a special prop, you should just pass it as a child like in normal reflex components, you don't have to specify the keyword argument.

For the styling - if you can create an issue for that, I think it's something we should be able to add.

seewindcn commented 2 months ago

yes, the code works; my question is about the index.js which generate from python:

 export function Layout1_cf15c797d04480712961c3b3fa2f9a5e () {
const state__state = useContext(StateContexts.state__state)                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                             
return (                                                                                                                                                                                                    
<Layout1 children={["<RadixThemesFlex .................................here is ..useless...........................................>
   here is ok.
</Layout1>

"<Layout1 children=" this children prop is useless, and maybe very very long. it may js file's size bigger.

picklelo commented 2 months ago

@seewindcn Got it - I think this is just a bug in our memo, we should be recognizing children is not a normal prop, and it should put it within the children. We can keep this ticket open to track this, but seems like it's not a hard blocker at the moment.