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

[Card] Expand on Click #1806

Merged
merged 4 commits into from
Oct 7, 2015
Merged

[Card] Expand on Click #1806

merged 4 commits into from
Oct 7, 2015

Conversation

codeheroics
Copy link
Contributor

This is an implementation proposal for #1354 : To be able to expand card by clicking on an element such as CardHeader, rather than only being able to do it using the CardExpandable button.

This adds a new expander prop for Card child components, which triggers the expand on click.

I also updated the card documentation as to provide a demo of the proposed use.

@codeheroics
Copy link
Contributor Author

Just did a fixup on the first commit, if anyone tried the previous version I had forgotten to commit modifications and it didn't work, sorry about that.

@shaurya947
Copy link
Contributor

@hugo-agbonon I assume that the expander prop is used to determine whether or not the card can be expanded by clicking this child. Correct me if I'm wrong. If that's the case, maybe change the prop name to something more intuitive? Like canActAsExpander?

Otherwise, this looks fine to me from a high-level (yet to run the code though). @droberts84 @MichaelTaylor3D you guys have any feedback on this?

@@ -100,7 +93,7 @@ const CardExpandable = React.createClass({
let expandableBtn = (
<IconButton
style={mergedStyles}
onClick={this._onExpanding}>
onClick={this.props.onExpanding}>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we have a onClick and not a onTouchTap?

@codeheroics
Copy link
Contributor Author

I replaced the onClick events by onTouchTap events, and renamed expander to actAsExpander

@shaurya947 That's right, any direct child component of Card with the prop actAsExpander set to true will cause the card to be expanded on a click of this child.

@shaurya947
Copy link
Contributor

Sounds good @hugo-agbonon! Thanks!

shaurya947 added a commit that referenced this pull request Oct 7, 2015
@shaurya947 shaurya947 merged commit 58685dc into mui:master Oct 7, 2015
@codeheroics codeheroics deleted the feat-cardExpander branch October 13, 2015 17:32
@@ -31,6 +32,10 @@ const Card = React.createClass({
}
if (this.state.expanded === false && currentChild.props.expandable === true)
return;
if (currentChild.props.actAsExpander === true) {
currentChild.props.onTouchTap = this._onExpandable;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hugo-agbonon this will not work with React 0.14. Props can no longer be mutated.. see here: https://facebook.github.io/react/blog/2015/10/07/react-v0.14.html#breaking-changes

I'll try to fix this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shaurya947 @cgestes already wrote a fix, see #1868 / #1872,

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad, missed that. Merged now.

@zannager zannager added the component: card This is the name of the generic UI component, not the React module! label Mar 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: card This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants