DeloitteAU / react-habitat

⚛️ 🛅 A React DOM Bootstrapper designed to harmonise a hybrid 'CMS + React' application.
Other
262 stars 42 forks source link

Bug when used with React 16 and Redux? #27

Closed ryanpcmcquen closed 6 years ago

ryanpcmcquen commented 6 years ago
Warning: Invalid value for prop `dispatch` on <img> tag. Either remove it from the element, or pass a string or number value to keep it in the DOM. For details, see https://fb.me/react-attribute-behavior

Seeing this error when using React Habitat and React Habitat Redux with React 16. Is it because of this change? https://reactjs.org/blog/2017/09/08/dom-attributes-in-react-16.html

jenssogaard commented 6 years ago

@ryanpcmcquen I don't believe that your error and the change you are referring to are related. I interpret your error message as relating to the value being passed. Would you mind sharing relevant parts of your code?

ryanpcmcquen commented 6 years ago

Sure, but I am not passing a dispatch prop anywhere ...

jennasalau commented 6 years ago

I have a feeling this is redux related. Your code would be handy so we can replicate it 😃

ryanpcmcquen commented 6 years ago

@jennasalau, I think so too, moving to a public repo now ...

ryanpcmcquen commented 6 years ago

Sorry for the delay, here it is: https://github.com/ryanpcmcquen/react-habitat-poc

jennasalau commented 6 years ago

Thanks Ryan, Im out n about this weekend so won't be able have a look until Monday. Unless @jenssogaard spots something untoward.

jenssogaard commented 6 years ago

Hi @ryanpcmcquen. Did a clone and the issue is that your Card props contain Redux references which obviously cannot be added as DOM-props to the img-tag. I suggest that you either add your props explicitly <img src="{src}" />. Alternatively, do a clone of props excluding dispatch, but I think the first option is better.

ryanpcmcquen commented 6 years ago

Thank you, I will try that. There is not a dispatch prop though ... and it works fine outside of Habitat.

jenssogaard commented 6 years ago

Your welcome. But there is a dispatch prop: https://www.dropbox.com/s/sml4fi2y5yum60j/Screenshot%202018-01-13%2021.04.46.png?dl=0

ryanpcmcquen commented 6 years ago

Ah, Redux must create that.

jennasalau commented 6 years ago

Yuppers, Redux connect does it actually.

I wouldn't recommend putting a props spread on an image tag like that anyway.

for example i feel instead of this

<img {...props} />

This would be better

<img alt={props.alt} src={props.src} />
ryanpcmcquen commented 6 years ago

Yeah, I will refactor it and close this once I test it. 😃

jennasalau commented 6 years ago

Awesome.. hopefully we got to the bottom of it! Thanks for raising! :) and thanks for your help @jenssogaard

ryanpcmcquen commented 6 years ago

OK, sweet! @jennasalau & @jenssogaard, thank you so much. The only thing that is not working, is that the addToCart prop is not passing through (so the ADD TO CART button is not being created). Any ideas on what is causing that?

It does work if I lowercase the attribute (addToCart => addtocart). Do all props need to be lowercase for compatibility with Habitat?

jennasalau commented 6 years ago

Javascript unfortunately only reads HTML attributes in lowercase as per spec.

ie <div data-addToCart="true /> is actually seen as <div data-addtocart="true" /> when you read it with JS (which is what Habitat does). There is a good discussion on it here.

For this reason, habitat converts hyphen separated case to camelCase so you can keep javascript code style.

So Habitat converts <div data-prop-add-to-cart="true" /> to props.addToCart = true.

I did my best to explain it here under heading "prefix".

Does this help or have i miss understood you?

ryanpcmcquen commented 6 years ago

@jennasalau, that helps a ton. Youse guys rock!