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

Bugfixes: Allow base props "className" and "style" to work on all components, fix some event listener issues. #3

Merged
merged 6 commits into from
Apr 27, 2020

Conversation

weltraumpirat
Copy link
Contributor

Hi, this is a workaround to fix an issue that prevents className and style base properties from being forwarded to web components. See github.com/facebook/react/issues/4933

I noticed that there was no way I could style individual components, so I figured out a way to make it work.

I have tried to preserve the styling as well as I could, but suppressed a few warnings about empty tags in tests.

…perties from being forwarded to web components. See github.com/facebook/react/issues/4933
…ltiplication of handlers - one eventlistener is added per render pass - and thus, a single click will cause many actions, e.g. when a form is updated via onChange() on input elements.
…ltiplication of handlers - one eventlistener is added per render pass - and thus, a single click will cause many actions, e.g. when a form is updated via onChange() on input elements.
@weltraumpirat weltraumpirat changed the title Allow base props "className" and "style" to work on all components Bugfixes: Allow base props "className" and "style" to work on all components, fix some event listener issues. Apr 23, 2020
@just214
Copy link
Owner

just214 commented Apr 23, 2020

Thanks for all the pull requests! It looks like the tests are all passing. Did you test out each of your changes in Storybook to make sure everything works as expected?

@weltraumpirat
Copy link
Contributor Author

Hi Justin,

I did that just now: It seems to work fine.

I used the components in a showcase app I’m building. I found all the bugs that way, and fixed because I had to. They do work in that setting now -- except for an issue with the listbox that is also present in the web components themselves, that is ;)

Lmk if I can help out with other stuff. I like both the wired-elements and your implementation, and I'm probably going to be using it more, anyway.

@just214
Copy link
Owner

just214 commented Apr 23, 2020

Great, thanks again. I will comb through it a bit more, but everything looks good so far.

As far as helping, bug fixes are always great. I'm sure there are some bugs that I didn't find. So, if you find one, please feel free to file an issue and/or submit a PR with the fix. Also, feel free to put a link to your showcase in the README 😄

@weltraumpirat
Copy link
Contributor Author

Will do, once it is public :) Thanks for maintaining this - super useful!

@just214 just214 merged commit d63f595 into just214:master Apr 27, 2020
@just214
Copy link
Owner

just214 commented Apr 27, 2020

Good morning, I pulled in your changes and published a new 0.1.5 release with these changes. Thanks again for your contributions!

https://github.com/gojutin/react-wired-elements/releases/tag/v0.1.5

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