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][base] Add Tailwind CSS + plain CSS demo on the Button page #38240

Merged
merged 3 commits into from
Aug 7, 2023

Conversation

alisasanib
Copy link
Contributor

Part of #37222

@mui-bot
Copy link

mui-bot commented Jul 30, 2023

Netlify deploy preview

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 2249cdf

@mj12albert mj12albert added docs Improvements or additions to the documentation package: base-ui Specific to @mui/base labels Jul 31, 2023
@mj12albert mj12albert requested a review from zanivan July 31, 2023 04:28
Comment on lines 11 to 12
<Button
className="cursor-pointer disabled:cursor-not-allowed text-sm bg-violet-500 text-white rounded-md font-semibold px-4 py-2 border-none disabled:opacity-50"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<Button
className="cursor-pointer disabled:cursor-not-allowed text-sm bg-violet-500 text-white rounded-md font-semibold px-4 py-2 border-none disabled:opacity-50"
<Button
className="cursor-pointer disabled:cursor-not-allowed text-sm bg-violet-500 text-white rounded-md font-semibold px-4 py-2 border-none disabled:opacity-50"

We discussed that it would probably be better if we create a CustomButton instead of adding all classes inline in each button's instance. For e.g.

const CustomButton = React.forwardRef((props, ref) => {
  const { className, ...other } = props;
  return (
    <Button className={clsx("cursor-pointer disabled:cursor-not-allowed text-sm bg-violet-500 text-white rounded-md font-semibold px-4 py-2 border-none disabled:opacity-50", className)} {...other} ref={ref} />
  )
});

I would propose to update all demos following this pattern. It's closer to how developers would do it in their project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @mnajdova for the explanation. I pushed the changes that you suggested. I'll continue to work on other demos and components if you agree with the implementation :)

@alisasanib alisasanib marked this pull request as draft July 31, 2023 16:42
@alisasanib alisasanib marked this pull request as ready for review August 1, 2023 08:55
@alisasanib alisasanib requested a review from mnajdova August 1, 2023 08:56
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

Excellent 👍 Nice job! In terms of order, I feel that the biggest impact we would make if we move the Introduction demos first on each components page, then we can slowly add the switcher to the other demos too :)

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 3, 2023
@alisasanib alisasanib force-pushed the base-button-tailwind-plain-css branch from ab0a1da to 2249cdf Compare August 4, 2023 14:57
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 4, 2023
@mnajdova mnajdova merged commit d07a256 into mui:master Aug 7, 2023
@alisasanib alisasanib deleted the base-button-tailwind-plain-css branch August 7, 2023 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation package: base-ui Specific to @mui/base
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants