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

D-14522 #13

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@andrew-codes
Copy link
Collaborator

andrew-codes commented Feb 11, 2019

No description provided.

andrew-codes added some commits Feb 11, 2019

focusedIndex,
};
class Accordion extends Component {
constructor() {

This comment has been minimized.

@corywheeler

corywheeler Feb 11, 2019

Member

Noticed props and context dropped from the constructor here and didn't get passed to super. Thought that those were necessary/good practice for React components.

const clonedChild = React.cloneElement(child, childProps);
return clonedChild;
})}
</Fragment>

This comment has been minimized.

@corywheeler

corywheeler Feb 11, 2019

Member

This section of code confuses me, can we talk about this space?

);
return { isOpenByIndex };
}

This comment has been minimized.

@corywheeler

corywheeler Feb 11, 2019

Member

Would like to chat about this section as well.

@walkerrandolphsmith

This comment has been minimized.

Copy link
Contributor

walkerrandolphsmith commented on packages/components/src/Accordion/Item.js in bfb398d Feb 12, 2019

wow lol... That literally just makes childProps == otherProps and then clones child with them....

@walkerrandolphsmith walkerrandolphsmith deleted the D-14522 branch Feb 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment