-
Notifications
You must be signed in to change notification settings - Fork 143
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
Implement basic support for adding maps to SCNMaterial #121
Conversation
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.
can you do some renaming?
@@ -69,20 +69,19 @@ export default (mountConfig, propTypes = {}, nonUpdateablePropKeys = []) => { | |||
// any custom props (material, shape, ...) | |||
const nonNodePropKeys = keys(propTypes); | |||
|
|||
const processColors = props => ({ | |||
const parseMaterials = props => ({ |
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.
there is a file called "parseMaterial", but exported functions have completly different names.
But here you have function "parseMaterials". I know it was already confusing to begin with, but maybe we can improve here
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.
I renamed it to processMaterial
to match the processColor
function we are already using
components/lib/parseMaterial.js
Outdated
}, | ||
}), | ||
material, | ||
); |
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.
- it took my a while to get that. Can you rename the variables? Also maybe an approach without reduce would be more easy to understand. maybe with mapValues from lodash?
- also a comment about why we support diffuse to be a string.
- instead of typeof you can use
isString(prevMaterial[property])
This PR allows for materials to set with maps like-