Skip to content

Chakra v3 support #805

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

Merged
merged 19 commits into from
Nov 8, 2024
Merged

Chakra v3 support #805

merged 19 commits into from
Nov 8, 2024

Conversation

abrenoch
Copy link
Contributor

@abrenoch abrenoch commented Oct 28, 2024

Copy of #804

Closes #802

Copy link

netlify bot commented Oct 28, 2024

Deploy Preview for react-querybuilder ready!

Name Link
🔨 Latest commit 0ca9c3a
🔍 Latest deploy log https://app.netlify.com/sites/react-querybuilder/deploys/67294562b7a079000835fcd1
😎 Deploy Preview https://deploy-preview-805--react-querybuilder.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 66 (🔴 down 2 from production)
Accessibility: 89 (no change from production)
Best Practices: 100 (no change from production)
SEO: 95 (no change from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

@jakeboone02
Copy link
Member

Looks like I can edit this one. Thanks!

@jakeboone02
Copy link
Member

Well, it says I can push to it on the PR, but it won't actually let me push.

image

> git push abrenoch HEAD:chakra-v3
remote: Permission to abrenoch/react-querybuilder.git denied to jakeboone02.
fatal: unable to access 'https://github.com/abrenoch/react-querybuilder/': The requested URL returned error: 403

Could there be a setting on your account or the fork that might be causing that?

@jakeboone02
Copy link
Member

I wonder if it's because this PR's branch was forked from that org branch and not directly from this repo:

image

@abrenoch
Copy link
Contributor Author

Possibly! How silly. I tried adding you as a collaborator to my repo if that helped?

@jakeboone02
Copy link
Member

Possibly! How silly. I tried adding you as a collaborator to my repo if that helped?

Yep, that did it. Thanks!

Copy link

codesandbox-ci bot commented Oct 29, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 0ca9c3a:

Sandbox Source
React Query Builder CI Template Configuration
React Query Builder Basic Template Configuration
React Query Builder Basic TypeScript Template Configuration
React Query Builder Drag-and-drop Template Configuration
React Query Builder Ant Design Template Configuration
React Query Builder Bootstrap Template Configuration
React Query Builder Bulma Template Configuration
React Query Builder Chakra UI Template Configuration
React Query Builder Fluent UI Template Configuration
React Query Builder Mantine Template Configuration
React Query Builder MUI/Material Template Configuration
React Query Builder Native Template Configuration
React Query Builder Next.js Template Configuration
React Query Builder Tremor Template Configuration

Copy link

codecov bot commented Oct 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (1bb5c5f) to head (0ca9c3a).
Report is 40 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #805   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          154       153    -1     
  Lines         5294      5290    -4     
  Branches      2596      2650   +54     
=========================================
- Hits          5294      5290    -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jakeboone02
Copy link
Member

@abrenoch I updated all the "other" stuff like the dev page, example, and website demo. Want to give 'em a whirl?

I'm still not sure how I'm going to handle dual compatibility with v2, but let me know what you think of these changes for v3.

@abrenoch
Copy link
Contributor Author

@jakeboone02 Passes the sniff test for me, I think you nailed it!

@abrenoch abrenoch marked this pull request as ready for review October 30, 2024 13:43
@jakeboone02
Copy link
Member

Great! I'll merge this once I figure out what to do about v2 going forward.

@jakeboone02
Copy link
Member

Hey, one question: Do you think the "snippets" are really necessary in the case of a library like this? I know it's the recommended way in the Chakra docs, but they're not really re-used in the way an application would re-use them. Each snippet is only ever imported once within the package (so the props are always the same), and I feel like the snippet code could just be integrated directly into the existing component files or even removed completely in some cases. For example, I don't see a need for the "loading" render paths in snippets/button, so we could just use Button directly from @chakra-ui/react in ChakraActionElement (like ChakraShiftActions already does).

@abrenoch
Copy link
Contributor Author

Hey, one question: Do you think the "snippets" are really necessary in the case of a library like this? I know it's the recommended way in the Chakra docs, but they're not really re-used in the way an application would re-use them. Each snippet is only ever imported once within the package (so the props are always the same), and I feel like the snippet code could just be integrated directly into the existing component files or even removed completely in some cases. For example, I don't see a need for the "loading" render paths in snippets/button, so we could just use Button directly from @chakra-ui/react in ChakraActionElement (like ChakraShiftActions already does).

I can see this making sense, especially in the case of Button which chakra still seems to supply a more primitive version of. I was thinking about that when making the changes, but since I'm not super familiar with this library I wanted to keep the chakra component APIs as close to what they originally where for compatibility reasons. I think that would be a sensible approach!

@jakeboone02
Copy link
Member

Thanks for the feedback. After looking it over a little more, I decided to stick with the snippets after all, except button and the unused field snippet.

@jakeboone02 jakeboone02 merged commit e0070bb into react-querybuilder:main Nov 8, 2024
14 checks passed
@jakeboone02
Copy link
Member

@all-contributors please add @abrenoch for code

Copy link
Contributor

@jakeboone02

I've put up a pull request to add @abrenoch! 🎉

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.

Support Chakra v3
2 participants