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

Switch from radix to blueprint #780

Merged
merged 26 commits into from
Dec 6, 2024
Merged

Conversation

wadjih-bencheikh18
Copy link
Contributor

@wadjih-bencheikh18 wadjih-bencheikh18 commented Oct 23, 2024

closes : #469

For the RadioGroup component, I created the same we have with BluePrintJs
Either we update ours and make it use BluePrintJs instead of radix or just remove the component because it can be done using BluePrintJs

Technically I prefer we update ours because recreating it with BluePrintJs is not simple.

https://switch-from-radix-to-bluepri.react-science.pages.dev/stories/?path=/story/forms-radio--control-button
https://switch-from-radix-to-bluepri.react-science.pages.dev/stories/?path=/story/components-infopanel--basic

@stropitek
Copy link
Contributor

stropitek commented Oct 24, 2024

Technically I prefer we update ours because recreating it with BluePrintJs is not simple.

Yes let's have our own component using the style override you made. Let's make it such that it has the exact same API as the blueprint RadioGroup component. Also let's name it differently to avoid name clashes: RadioButtonGroup

Copy link
Contributor

@stropitek stropitek left a comment

Choose a reason for hiding this comment

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

Make sure to remove any non-used packages.

Do not change anything in the design of the component. I see a background color change on InfoPanel. Double check for other differences.

Copy link

cloudflare-workers-and-pages bot commented Oct 24, 2024

Deploying react-science with  Cloudflare Pages  Cloudflare Pages

Latest commit: 55ccf2f
Status: ✅  Deploy successful!
Preview URL: https://ea0bb445.react-science.pages.dev
Branch Preview URL: https://switch-from-radix-to-bluepri.react-science.pages.dev

View logs

@wadjih-bencheikh18
Copy link
Contributor Author

@stropitek this PR is ready

@stropitek
Copy link
Contributor

The hovered style of radio buttons is missing (vs. on the main branch)

Copy link
Contributor

@stropitek stropitek left a comment

Choose a reason for hiding this comment

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

Please make the height and font size of the radio button component similar to other form components.

  • Regular: 30px height and 14px font size (using rem unit)
  • Large: 40px height and 16px font size (using rem unit)

Please provide a commit message with the list of breaking change.

Copy link
Contributor

@stropitek stropitek left a comment

Choose a reason for hiding this comment

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

There is a regression in the sticky accordion headers of the info panel.

CleanShot.2024-11-04.at.09.04.06.mp4

CleanShot 2024-11-04 at 09 17 35@2x

The height should be 30px / 40px including the borders. It should be the same size next to a text input.

Do your commit messages include all the breaking changes or will you provide a commit message with all the breaking changes in the PR thread?

@wadjih-bencheikh18
Copy link
Contributor Author

Do your commit messages include all the breaking changes or will you provide a commit message with all the breaking changes in the PR thread?

They Include all breaking changes

@wadjih-bencheikh18
Copy link
Contributor Author

The height should be 30px / 40px including the borders. It should be the same size next to a text input.

Should border-radius also be the same?

@stropitek
Copy link
Contributor

Should border-radius also be the same?

No does not have to be

@stropitek
Copy link
Contributor

The content does not look centered now (it's too low), and there is a double border on the selected item.

CleanShot 2024-11-05 at 10 52 24@2x

@stropitek
Copy link
Contributor

ping @wadjih-bencheikh18

Will you be able to finish this?

@stropitek
Copy link
Contributor

@wadjih-bencheikh18 I am taking this over.

@stropitek stropitek marked this pull request as draft December 3, 2024 13:00
@stropitek stropitek force-pushed the switch-from-radix-to-blueprint branch from e2d3320 to d7f13bf Compare December 3, 2024 13:01
@stropitek
Copy link
Contributor

Commit message

refactor: switch from radix to blueprint

BREAKING CHANGE: The `RadioGroup` component has been renamed to `RadioButtonGroup`. Its interface is now the same as blueprintjs's `RadioGroup` component, which changes how multiple of its props are named and typed.

@stropitek stropitek marked this pull request as ready for review December 4, 2024 10:51
@stropitek stropitek requested a review from targos December 4, 2024 10:52
@targos
Copy link
Member

targos commented Dec 5, 2024

Shouldn't this uninstall the radix dependency?

@stropitek stropitek marked this pull request as draft December 6, 2024 11:01
@stropitek stropitek marked this pull request as ready for review December 6, 2024 11:30
@stropitek
Copy link
Contributor

Yes they are no longer used, I removed them.

@stropitek stropitek merged commit d69d7e0 into main Dec 6, 2024
12 checks passed
@stropitek stropitek deleted the switch-from-radix-to-blueprint branch December 6, 2024 13:27
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.

Switch components from headlessui or radix to blueprint
3 participants