Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: ClientOnly documentation #1289

Merged
merged 9 commits into from
Nov 28, 2023
Merged

docs: ClientOnly documentation #1289

merged 9 commits into from
Nov 28, 2023

Conversation

usk94
Copy link
Member

@usk94 usk94 commented Nov 25, 2023

ref: vikejs/vike-react#35

For ClientOnly documentation.
I also made some changes to the description on the client-only-components page, but please feel free to let me know if these are unnecessary.

@brillout
Copy link
Member

👌 One thing: most (and many will never) use vike-react so I think it would be nice to keep the <ClientOnly> implementation example in the docs for these users. I guess we can simply add a note with a <Link href="/ClientOnly" />.

Btw. I reverted prettier; the docs don't use prettier, unfortunately so: we should apply prettier to the docs at some point :)

@usk94
Copy link
Member Author

usk94 commented Nov 26, 2023

@brillout

One thing: most (and many will never) use vike-react so I think it would be nice to keep the implementation example in the docs for these users. I guess we can simply add a note with a .

Got it, sorry about that, I didn't understand the difference between the scope of vike-react and the scope of react in vike.
How about this fix? 5c6e545

@brillout
Copy link
Member

I wonder whether we should document vike-react/ClientOnly and vike-solid/ClientOnly at the same time or separately 🤔 (In the long term a global toggle vike-react/vike-solid/vike-vue would be neat.)

Got it, sorry about that, I didn't understand the difference between the scope of vike-react and the scope of react in vike.
How about this fix? 5c6e545

👍

@brillout
Copy link
Member

I wonder whether we should document vike-react/ClientOnly and vike-solid/ClientOnly at the same time or separately 🤔 (In the long term a global toggle vike-react/vike-solid/vike-vue would be neat.)

I think it's easier if we have separate sections for each:

{/* pages/ClientOnly.page.server.mdx */}

## React

...

## Solid

...

It's also a better fit if we eventually implement a UI framework toggle.

@usk94
Copy link
Member Author

usk94 commented Nov 27, 2023

The core logic of each should be mostly the same, so I think it would be nice to first explain the concept of ClientOnly, and then show how it can be implemented in each UI framework.

I think it's easier if we have separate sections for each:

So, I also believe that the good immediate approach would be to separate each section.

@brillout
Copy link
Member

The core logic of each should be mostly the same, so I think it would be nice to first explain the concept of ClientOnly, and then show how it can be implemented in each UI framework.

👍

@usk94
Copy link
Member Author

usk94 commented Nov 27, 2023

Should I also write an example implementation for Solid?😀

I will add this example for Solid with this PR😀

import { ClientOnly } from "vike-solid/ClientOnly";

function MyComponent() {
  return (
    <ClientOnly load={() => import('path-to-dynamic-component')} fallback={<Loading />}>
      {(Component) => <Component {...props} />}
    </ClientOnly>
  );
}

@usk94
Copy link
Member Author

usk94 commented Nov 27, 2023

@brillout
I've added the explanation for Solid, but does this look good?
I'm concerned about the redundancy of repeating the same description in the Props section.

@brillout
Copy link
Member

I've added the explanation for Solid, but does this look good?

👍

I'm concerned about the redundancy of repeating the same description in the Props section.

How about creating a new component at pages/ClientOnly/ClientOnlyCommon.tsx? We do this quite a lot to avoid redundancy.

@usk94
Copy link
Member Author

usk94 commented Nov 27, 2023

@brillout
Thank you!🙌

How about creating a new component at pages/ClientOnly/ClientOnlyCommon.tsx? We do this quite a lot to avoid redundancy.

I just want to confirm: does this mean we should create a pages/ClientOnly/ClientOnlyCommon.page.server.mdx for the common content and then have separate pages for React and Solid specifics?

@brillout
Copy link
Member

I was thinking more of something around these lines:

{/* pages/ClientOnly.page.server.mdx */}

import { ClientOnlyCommon } from './ClientOnly/ClientOnlyCommon.tsx'

## React

...

<ClientOnlyCommon packageName="vike-react" hasDepsArgument={true} ... />

...

## Solid

...

<ClientOnlyCommon packageName="vike-solid" hasDepsArgument={false} ... />

...

I think a single page vike.dev/ClientOnly is future proof (feel free to disagree).

@usk94
Copy link
Member Author

usk94 commented Nov 28, 2023

Thank you for letting me know and it's great the way.

Do I need to worry about syntax highlight?

@brillout
Copy link
Member

Do I need to worry about syntax highlight?

Usually what I do is that I define code blocks in small mdx files SomeCodeBlock.mdx and then import it from MyComponent.tsx.

But that doesn't work for code blocks with conditional content, so I'm thinking we can create a new component <CodeBlock lang="tsx">...<CodeBlock/> at docs/components/CodeBlock.ts that generates the same <pre><code ...>{children}</code></pre> wrapper that mdx generates. Actually that isn't enough. So how about we create two versions ClientOnlyExampleWithDeps.mdx and ClientOnlyExampleWithoutDeps.mdx and use ClientOnlyCommon.tsx to conditionally use the right one?

@usk94
Copy link
Member Author

usk94 commented Nov 28, 2023

Thanks for letting me know.
This might be a bit different from what you're envisioning, but how about this fix?

@brillout
Copy link
Member

This might be a bit different from what you're envisioning, but how about this fix?

Perfect 👌

I made some extra polishing; let me know if you disagree with any of it, otherwise I'll merge.

I moved the page to headingsDetached but I'll add it to the API section once the V1 docs PR is merged.

@usk94
Copy link
Member Author

usk94 commented Nov 28, 2023

Thank you for polishing.

I made some extra polishing; let me know if you disagree with any of it, otherwise I'll merge.

No problem🙆‍♂️

I moved the page to headingsDetached but I'll add it to the API section once the V1 docs PR is merged.

Understood. I'm looking forward to it😀

@brillout brillout merged commit 343492d into vikejs:main Nov 28, 2023
15 checks passed
brillout added a commit that referenced this pull request Nov 28, 2023
@brillout
Copy link
Member

Merged 🎉

I'll add it to the API section

Done: 076d17f.

Neat PR again 👌

Let me know if something in contribution-welcome 💕 piques your interest 👀

Feel free to PM us on Discord (I tried to find you as usk94 in Discord but couldn't find you).

@usk94 usk94 deleted the docs/client-only branch November 28, 2023 13:02
@usk94
Copy link
Member Author

usk94 commented Nov 28, 2023

Done: 076d17f.

Thanks!

I tried to find you as usk94 in Discord but couldn't find you

Oops, I was using my real name. I just renamed it to usk94👌

Feel free to PM us on Discord

I'm glad to hear that. Thanks a lot😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants