-
Notifications
You must be signed in to change notification settings - Fork 88
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
Build/react wrapper #85
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/mux/media-chrome/dXMPndGfpV3Ds9StYF55aqLUkwb3 |
42703c1
to
d1a548e
Compare
d1a548e
to
7ff8601
Compare
72ce966
to
8d8cd12
Compare
8d8cd12
to
164bb7c
Compare
@@ -0,0 +1,54 @@ | |||
const ReactPropToAttrNameMap = { | |||
className: 'class', | |||
classname: 'class', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
Reviewing the code and playing around with this, here's what I'm finding: ✅ I'm on board with this pattern:
It would be nice if it wasn't nested under ✅ I'm able to follow the generator code and what's going on there in order to make this happen QA'ing this was a bit difficult, as I couldn't get this working in a simple examples/ file. In order to QA i created a next.js app and imported the ES Modules via a file URL, which is supported in Next 12 It worked, and I got a player 🎉 Open questions: ❓ In your example @mmcc pinging you to get some feedback here since Heff is out. TLDR is:
|
What's left to do on this one to get it merged? Any new ideas to apply from the research of how others are handling it? |
@cjpillsbury looking good -- tested out and was able to get a nextjs app in examples/ (excluding nextjs example from the publishing step, via Following up on my open questions above:
Discussed synchronously, we're changing the behavior to use camel case convention
A "usage in React" section that links to another
Done
Would be good to have some test coverage on this stuff
Not worried about this right now
Opening a new discussion, we would basically need this to generate proper TS support: #158 |
164bb7c
to
72998ed
Compare
…from react props to native html attributes.
…. (WIP DO NOT MERGE).
…ses/seams in build code.
…d react wrapper build to npm scripts.
…handle SSR cases. Add jsdoc types for ts/tsx usage.
… type declarations file generation.
f0ab99d
to
4529733
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
First pass implementation of DIY React wrapper "compiler" that creates React component wrappers for each identified custom element to reduce friction for React devs working with media-chrome.
Also updates
media-chrome
to correctly identify itself as"type": "module"
per new node/npm ESM support.To Build/Use:
yarn build
yarn build:react
./dist/react/*
To test (for the PR before the release):
create-react-app
media-chrome
as a dependencymedia-chrome
if you haven't already, check out this branch (you will have to set up my fork as an additional remote), andnpm link
media-chrome to the newly-created react app project.Some problems it solves:
Example Usage demonstrating different "react-expected" behaviors:
(Partial) Example of "compiled" wrapper module: