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

Feat/remix recipe #227

Merged
merged 10 commits into from
Nov 23, 2023
Merged

Conversation

EarthlingDavey
Copy link
Contributor

@EarthlingDavey EarthlingDavey commented Nov 14, 2023

Add remix recipe - I've only touched the remix/recipe folder. So I don't know if there's extra stuff I need to do to prepare the package for release.

It's working fine - but with some TS errors like : #185

Do you mind reviewing, and advising the best way to fix the errors.

I can also remove or amend the devcontainer if needed.

Thanks

Copy link

vercel bot commented Nov 14, 2023

@EarthlingDavey is attempting to deploy a commit to the Measured Team on Vercel.

A member of the Team first needs to authorize it.

@EarthlingDavey
Copy link
Contributor Author

I'm not sure why, but @measured/puck-plugin-heading-analyzer. Should we try and get that working in this PR or put off and create an issue?

Copy link
Member

@chrisvxd chrisvxd left a comment

Choose a reason for hiding this comment

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

Thanks @EarthlingDavey! This works beautifully 🙌

I've left some comments.

To get this merged, you'll also need to add the recipe to the create-puck-app templates. You don't need to add all the code, just any handlebars overrides (which will include the package.json - see the next example).

Would also appreciate it if you could add it to the README, below the next recipe.

.devcontainer/devcontainer.json Outdated Show resolved Hide resolved
// crypto is used by packages/core/dist/index.js
// is it possible to not send it to the browser?
// for not, we need to polyfill it...
browserNodeBuiltinsPolyfill: { modules: { crypto: true } },
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit awkward. We can hopefully remove this when we deal with #112

*/
// const matches = useMatches();
// const includeScripts = matches.some((match) => match.handle?.hydrate);

Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit 50/50 about whether we should include this. It's great, but also bloats the recipe for users that want JS.

Let me have a think on it before we remove it.

recipes/remix/app/routes/$puckPath_.edit.tsx Show resolved Hide resolved
recipes/remix/app/routes/edit.tsx Outdated Show resolved Hide resolved
recipes/remix/app/routes/edit.tsx Outdated Show resolved Hide resolved
recipes/remix/app/routes/edit.tsx Outdated Show resolved Hide resolved
recipes/remix/app/routes/edit.tsx Outdated Show resolved Hide resolved
recipes/remix/app/routes/_index.tsx Outdated Show resolved Hide resolved
recipes/remix/app/routes/_index.tsx Show resolved Hide resolved
View: Untitled Page to Page.
Edit & View: Fix TS error
Copy link

vercel bot commented Nov 22, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
puck-demo ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 23, 2023 10:40am
puck-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 23, 2023 10:40am

remix.config.js update comment re: crypto
@EarthlingDavey
Copy link
Contributor Author

@chrisvxd thanks for the review. I've worked through them and not touched the disable-js code. I'm not sure about that too. Perhaps that should be stripped but added to an example codebase?

@chrisvxd
Copy link
Member

Thanks @EarthlingDavey! I think let's remove the no JS code. The user can figure that out based on the Remix docs.

Copy link
Member

@chrisvxd chrisvxd left a comment

Choose a reason for hiding this comment

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

Awesome stuff, thanks @EarthlingDavey. Merging now and will remove the noJS stuff myself in favour of the remix docs.

@chrisvxd
Copy link
Member

Can't be merged due to conflicts.

@chrisvxd chrisvxd mentioned this pull request Nov 23, 2023
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.

2 participants