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

new extension vike-react-chakra #151

Merged
merged 26 commits into from
Nov 17, 2024
Merged

Conversation

phonzammi
Copy link
Member

@phonzammi phonzammi commented Nov 4, 2024

TODO :

  • README

Add Custom Config/Setting :
ChakraCustomProvider ?
chakraCustomConfig ?
+system

chakra?: null | {
     system?: SystemContext
     locale?: LocaleProviderProps['locale']
}

@phonzammi phonzammi force-pushed the phonzammi/feat-vike-react-chakra branch from f7b00ae to dd2e0bd Compare November 4, 2024 16:55
@brillout
Copy link
Member

brillout commented Nov 4, 2024

💯

Hi Muhammad, yes good point I think a new setting +ChakraProvider would be great. But let me know if you think an alternative idea could be better.

Looking forward to merge 😍

@phonzammi phonzammi force-pushed the phonzammi/feat-vike-react-chakra branch from dd2e0bd to c403cbf Compare November 5, 2024 16:39
@phonzammi
Copy link
Member Author

phonzammi commented Nov 5, 2024

Hi Rom, Thanks
I just added +system setting and an example.

I'll also try adding the +ChakraProvider setting, which would allow users to use a custom provider, like adding ThemeProvider from next-themes. However, I think users can also achieve this by using the +Wrapper setting.

@brillout
Copy link
Member

brillout commented Nov 6, 2024

Hello Muhammad, nice 👌

I suggest we minimize the code a bit. The rationale being that we'll eventually have a lot of integration packages like this. To make things more maintainable let's keep things as simple as possible.

  • Let's remove the example.
  • Let's trim the readme a bit: let's assume the user already knows the basics of Chakra (but I ain't familiar with Chakra so I ain't sure what this means exactly).

I just added +system setting

👍 Is system the only prop of ChakraProvider? In other words: would replacing +system with +ChakraProvider make sense?

@brillout
Copy link
Member

brillout commented Nov 6, 2024

I'll also try adding the +ChakraProvider setting, which would allow users to use a custom provider, like adding ThemeProvider from next-themes

Ah, I'm just reading that 😅. So, yea, I'm inclined to think that haivng a +ChakraProvider setting would be both simpler and more powerful than having a +system setting?

I think users can also achieve this by using the +Wrapper setting.

Yea, maybe this integration package is a bit counter productive 🤔 WDYT?

Maybe with eject it could make sense?

@phonzammi
Copy link
Member Author

Is system the only prop of ChakraProvider?

Yes, it is, see ChakraProviderProps.

So, yea, I'm inclined to think that haivng a +ChakraProvider setting would be both simpler and more powerful than having a +system setting?

Yeah I think so. Users can configure the chakra theme system and add additional provider inside one +ChakraProvider.js file.

Yea, maybe this integration package is a bit counter productive 🤔 WDYT?

I'm not sure yet.

Maybe with eject it could make sense?

Indeed. Let's add +ChakraProvider setting first. And if we choose to keep the +system setting, maybe we can rename it to be +themeSystem or +chakraSystem.

@brillout
Copy link
Member

brillout commented Nov 6, 2024

I'm a bit confused: what would then be the added value of +ChakraProvider if system is the only setting?

I just looked at https://github.com/pacocoursey/next-themes; how is that related to (vike-react-)chakra?

@phonzammi
Copy link
Member Author

I'm not great at explaining this, but as far as I know, ChakraProvider only needs the theme system to work. The next-theme package isn’t necessary for it to function. However, the @chakra-ui/cli tool can adds code snippets that also installs the next-theme and react-icons packages, which are used in some of the pre-built component files generated by the CLI

@phonzammi phonzammi force-pushed the phonzammi/feat-vike-react-chakra branch from 609228f to c403cbf Compare November 7, 2024 03:36
@brillout
Copy link
Member

brillout commented Nov 7, 2024

However, the @chakra-ui/cli tool can adds code snippets that also installs the next-theme and react-icons packages, which are used in some of the pre-built component files generated by the CLI

How is this relevant to our discussion and/or to vike-react-chakra?

@phonzammi
Copy link
Member Author

So that users can follow the Setup Provider guide to use the generated Provider (which includes ThemeProvider) created by the CLI

@phonzammi phonzammi force-pushed the phonzammi/feat-vike-react-chakra branch from c403cbf to 2173a67 Compare November 7, 2024 15:09
@phonzammi
Copy link
Member Author

I just pushed a new changes.
Please take a look and review it

@phonzammi phonzammi marked this pull request as ready for review November 7, 2024 15:13
@phonzammi
Copy link
Member Author

I'll add @brillout/release-me package when we ready to merge it

@brillout
Copy link
Member

brillout commented Nov 8, 2024

So that users can follow the Setup Provider guide to use the generated Provider (which includes ThemeProvider) created by the CLI

Their docs say:

This provider composes the following:

  • ChakraProvider from @chakra-ui/react for the styling system
  • ThemeProvider from next-themes for color mode

But that isn't something the user can influence, right?

In other words: what would be the concrete added value of +ChakraProvider over +system?

Alternative idea:

// +chakra.js

export default {
  system: /* ... */
}

WDYT?

@phonzammi
Copy link
Member Author

But that isn't something the user can influence, right?

It's what the CLI generates, (inside ./src/components/ui dir), users can modify it as they want, or even create their own.

In other words: what would be the concrete added value of +ChakraProvider over +system?

  • If users use +ChakraProvider.js it's like overriding the vike-react-chakra's +Wrapper.js
  • +system.js only change/overriding the default chakra theme system configuration
  • If users choose to create a new +Wrapper.js to add ThemeProvider and also want to be able to change the chakra theme system, we need to keep +system setting.

@brillout
Copy link
Member

brillout commented Nov 9, 2024

It's what the CLI generates, (inside ./src/components/ui dir), users can modify it as they want, or even create their own.

I don't see how it's related to our discussion?

  • If users use +ChakraProvider.js it's like overriding the vike-react-chakra's +Wrapper.js

Sure, but again, what's the concrete advantage?

  • +system.js only change/overriding the default chakra theme system configuration

Of course.

  • If users choose to create a new +Wrapper.js to add ThemeProvider

Does their docs document that? 👀

I'm still not sure I see any concrete benefit of +ChakraProvider over +system.

@phonzammi
Copy link
Member Author

I don't see how it's related to our discussion?

Maybe I misunderstood your question. What do you mean by something the user can influence ?

@phonzammi
Copy link
Member Author

phonzammi commented Nov 9, 2024

Anyways, what about we don't use +ChakraProvider for now and use +system setting with your alternative idea.

// +chakra.js
 
export default {
   system: /* ... */
   locale: /* ... */
}

And we also can add optinal locale?: string too. Chakra UI has LocaleProvider component. So the final +Wrapper.ts file will look like this:

export { Wrapper }

import React, { ReactNode } from 'react'
import { ChakraProvider, LocaleProvider, defaultSystem } from '@chakra-ui/react'
import { usePageContext } from 'vike-react/usePageContext'

function Wrapper({ children }: { children: ReactNode }) {
  const pageContext = usePageContext()
  const { chakra } = pageContext.config

  return (
    <ChakraProvider value={chakra?.system ?? defaultSystem}>
      <ChakraLocaleProvider locale={chakra?.locale}>
        {children}
      </ChakraLocaleProvider>
    </ChakraProvider>
  )
}

function ChakraLocaleProvider(props: { locale?: string; children: ReactNode }) {
  if (props.locale) {
    return <LocaleProvider locale={props.locale}>{props.children}</LocaleProvider>
  }
  return <>{props.children}</>
}

@brillout
Copy link
Member

Sounds good!

Also, how about:

  • A default system? I don't know Chakra so I don't know if that can make sense.
  • When +chakra is set to null then no wrapper is added. Enabling the user to remove Chakra from certain pages and/or fully customize the Chakra integration.

@phonzammi phonzammi force-pushed the phonzammi/feat-vike-react-chakra branch from 2173a67 to 849fe84 Compare November 13, 2024 14:10
@phonzammi phonzammi force-pushed the phonzammi/feat-vike-react-chakra branch from 849fe84 to 0850332 Compare November 13, 2024 14:20
@phonzammi
Copy link
Member Author

  • A default system? I don't know Chakra so I don't know if that can make sense.

I'm not sure either, though I think the defaultSystem from Chakra UI is good enough to start with.

Okay, I'm happy with this PR. Please take a look and let me know if I missed anything.

Copy link
Member

@brillout brillout left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I made some polishing comments. As you can see I made a lot of them. The rationale here being that things will be ejectable so everything is part of the "shop window". Looking forward to it 💫

packages/vike-react-chakra/integration/+config.ts Outdated Show resolved Hide resolved
packages/vike-react-chakra/integration/Wrapper.tsx Outdated Show resolved Hide resolved
packages/vike-react-chakra/integration/Wrapper.tsx Outdated Show resolved Hide resolved
packages/vike-react-chakra/integration/Wrapper.tsx Outdated Show resolved Hide resolved
packages/vike-react-chakra/package.json Outdated Show resolved Hide resolved
packages/vike-react-chakra/README.md Outdated Show resolved Hide resolved
packages/vike-react-chakra/README.md Show resolved Hide resolved
packages/vike-react-chakra/README.md Outdated Show resolved Hide resolved
packages/vike-react-chakra/README.md Outdated Show resolved Hide resolved
packages/vike-react-chakra/README.md Outdated Show resolved Hide resolved
@brillout
Copy link
Member

It now works with [email protected].

@brillout brillout force-pushed the phonzammi/feat-vike-react-chakra branch from 64895ea to ff8eff3 Compare November 17, 2024 17:43
@brillout brillout force-pushed the phonzammi/feat-vike-react-chakra branch from 08dd9f9 to a9cb903 Compare November 17, 2024 17:46
@phonzammi
Copy link
Member Author

Well done, awesome 🚀 , thank you

@brillout
Copy link
Member

Alright, let's merge? Let me know if you disagree with anything.

@phonzammi
Copy link
Member Author

phonzammi commented Nov 17, 2024

LGTM, even better we don't need to use + for config.js.

FYI, I had issues with vike-solid-query+tsup because of that, sorry I didn't ask you before

@brillout
Copy link
Member

Made one last change: dbd7160.

@brillout brillout merged commit f3b5400 into main Nov 17, 2024
7 checks passed
@brillout brillout deleted the phonzammi/feat-vike-react-chakra branch November 17, 2024 18:16
@brillout brillout changed the title feat: new extension vike-react-chakra new extension vike-react-chakra Nov 17, 2024
@brillout
Copy link
Member

Merged ✨

Let's:

  • Release it
  • Add it to vike.dev/extensions
  • Create vike.dev/chakra

@brillout
Copy link
Member

Also we forgot to update the main readme: https://github.com/vikejs/vike-react

@brillout
Copy link
Member

brillout commented Nov 17, 2024

Also we forgot to update the main readme: https://github.com/vikejs/vike-react

Or better yet let's remove that readme and just add link to vike.dev/extensions#react

Monorepo of [Vike React extensions](...)

@phonzammi
Copy link
Member Author

Or better yet let's remove that readme and just add link to vike.dev/extensions#react

I agree

@brillout
Copy link
Member

Let's:

  • Release it
  • Add it to vike.dev/extensions
  • Create vike.dev/chakra

Also we forgot to update the main readme: https://github.com/vikejs/vike-react

Or better yet let's remove that readme and just add link to vike.dev/extensions#react

Monorepo of [Vike React extensions](...)

@phonzammi Up for it?

@phonzammi
Copy link
Member Author

Sure, I want to, ETA later tonight

@brillout
Copy link
Member

Ok ok, wasn't sure whether there was some ambiguity :)

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