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

[pigment-css][example] Add example project with Remix #41700

Merged
merged 2 commits into from
Apr 1, 2024

Conversation

brijeshb42
Copy link
Contributor

@brijeshb42 brijeshb42 commented Mar 29, 2024

@brijeshb42 brijeshb42 added examples Relating to /examples package: pigment-css Specific to @pigment-css/* labels Mar 29, 2024
@mui-bot
Copy link

mui-bot commented Mar 29, 2024

Netlify deploy preview

https://deploy-preview-41700--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 81066fd

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.

Let's revert the change to apps/pnpm-lock.yml, it's not related

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.

This reminds that we need to update all examples to use the next tag. I will handle this in a separate PR.

examples/pigment-css-remix-ts/package.json Outdated Show resolved Hide resolved
examples/pigment-css-remix-ts/package.json Outdated Show resolved Hide resolved
Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

I think Remix should be added to apps/pigment-css-remix-app too, it can be the same PR or a separate one, your call!

@mnajdova
Copy link
Member

I think Remix should be added to apps/pigment-css-remix-app too, it can be the same PR or a separate one, your call!

Agree! I will leave few questions here, even tough they are not 100% related to the PR:

  • should we add a CI step that build the apps/ projects and fails if it's not successful? We are very often breaking these examples while working on Pigment CSS, it could be a two steps validation that the changes work
  • how do we make sure that when we fix something in the apps/ directory we also update the examples if needed? I am not regularly checking if they are working, but taking into account how often we break the apps/ I won't be surprised if they are breaking from time to time with some changes

@brijeshb42
Copy link
Contributor Author

brijeshb42 commented Mar 29, 2024

should we add a CI step that build the apps/ projects

This is already the case and part of the current CI build. Both vite and next.js apps are built. The previous propTypes issue was failing in dev environment. And in most cases, the actual fix lies in the package code and rarely in the app code. So it should get autofixed since we always tag the latest release.

how do we make sure that when we fix something in the apps...

The example app is very minimal and doesn't cover complex use-case which is part of our current apps/ dir. But I agree that in future, when we expand the example apps to cover more use-cases, we'll also need to update the examples.

@brijeshb42
Copy link
Contributor Author

IMO, we can just convert the Vite app to a remix one. It'll provide coverage for both and we won't need to use the custom vite plugin for routing as Remix will already handle that.

@siriwatknp
Copy link
Member

IMO, we can just convert the Vite app to a remix one. It'll provide coverage for both and we won't need to use the custom vite plugin for routing as Remix will already handle that.

Nice!

Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

👍

@brijeshb42 brijeshb42 merged commit b338f3f into mui:next Apr 1, 2024
19 checks passed
@brijeshb42 brijeshb42 deleted the pigment/example-remix branch April 1, 2024 07:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
examples Relating to /examples package: pigment-css Specific to @pigment-css/*
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants