-
Notifications
You must be signed in to change notification settings - Fork 45
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
Migrate to Chakra V3 #752
base: main
Are you sure you want to change the base?
Migrate to Chakra V3 #752
Conversation
CloudFlare build failing:
|
Thanks for the log @humphd let me fix these. |
Use |
bump openai version Migrate Function to use Chakra 3.0 Fixing CodeHeader Style Issue Migrate Chakra3.0 to Mermaid Preview and Message Components fixing Modal Setting style | Promptform adjustment aligning | Sidebar content Function Section Adjust old toaster to use Chakra 3.0 Toaster | Comment ToastProgress Need to fix later
Deploying chatcraft-org with Cloudflare Pages
|
@humphd Preview some function might not be ready yet |
Add function for webhandlers and customization Fix recording microphone logical rendering toast progress comment out for use a chakra3.0 toast.progress
Logo color change Fixing Theme on DesktopPrompt
Fixing message themes Theme | Icons Fix bug new refresh page | theme icon | add badge Fixing Prompt Form mobile/dekstop view adjust theme in progress
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to keep this PR from affecting so much. You're changing a lot of things beyond the ChakraUI code, and this isn't a great idea, since it means this change becomes a) un-reviewable; b) very hard to test; c) likely to regress.
Can I get a better understanding of why you have to change so much?
@@ -44,8 +44,9 @@ | |||
"mammoth": "^1.8.0", | |||
"mermaid": "^11.4.0", | |||
"nanoid": "^5.0.8", | |||
"next-themes": "^0.4.3", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you introducing this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Chakra 3.0 is removed 'useColorMode' function so we have to use this one instead as they suggested in the documents.
ref https://www.chakra-ui.com/docs/get-started/migration#removed-features
"nomnoml": "^1.6.2", | ||
"openai": "^4.47.1", | ||
"openai": "^4.73.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unrelated to this change, please don't touch unrelated code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me go back and check why I think might relate tothat we change mode to Bundler instead of Node
@@ -1,6 +1,7 @@ | |||
import { lazy, memo, useMemo, type ReactNode } from "react"; | |||
import { Card, CardBody, IconButton } from "@chakra-ui/react"; | |||
import { CardRoot, CardBody, IconButton } from "@chakra-ui/react"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you switching away from Card?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We either can use object 'Card' and then Card.Root, also the same CardBody equalvalent to Card.Body
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A goal here should be to create the least amount of code churn possible: if you can do things without changing existing code, that's to be preferred.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The components we have been using in prvious version most of them cannot use here.
we have to changed the way to call components to match new version.
the components that create inder component/ui/.. are from new version.
Everything we have on Chakra2.8 cannot transfer to Chakra3.0, lots of them are removed, and all of the styling we have most of them need to change to make it align. In addition, major of the Chakra library is changed from we can call components from the modules, but now they created components for us to use it instead. 'alert' modal -> dialog All form removed -> Field and more |
I get that lots needs to change. But I want to end up with a change we can accept. If you change too much in one go, this is going to be impossible to land. Anything that can be done in follow-ups, I'd prefer you split out. Use your judgement (I'm not saying you're wrong), but I'm reading this PR and it makes me concerned. |
Absolutely, Im totally agree with that, this one need big change on our code base. From what I think we cannot even split to the small PR as well. |
From what I am thinking, I think how bout we create pr like this
@humphd let me know what you think is a go or need to plan further more. |
@fadingNA I think I want to pause on this work. You've done an awesome job so far, and it's possible that we'll want to end up going this way. But at the moment, we don't have the bandwidth to review and fix regressions in a PR this large. I think you've shown that the upgrade path to Chakra V3 is significant enough that we can basically consider it a rewrite, and if so, we should evaluate all the options. I'll leave this open in case we decide to restart the work in the near future. Thanks for your effort, and I'm sorry that we couldn't accept this right now. |
@humphd Hi @humphd - Thank you so much for the response. I'm totally understand this is a big change. Let's leave this PR open in case we really want to migrate to 3.0 I think this PR will be a good resource to check for reference as your mentioned . In addition, if we give it a go to migrate chakra ui to 3.0 I'm happy to contribute on that again in the near future. Non |
Move from fork repo PR to upstream repo.
Migrate to Chakra V3 #749
@humphd @Amnish04 @tarasglek