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

Allow resizing of CSF contained levels #13543

Merged
merged 5 commits into from Mar 6, 2017
Merged

Conversation

balderdash
Copy link
Contributor

The HeightResizer actually works now (it used to be fixed in place, since both the minHeight and maxHeight were set to the height of the contained level), with scrolling when you make it smaller.

Copy link
Contributor

@Hamms Hamms left a comment

Choose a reason for hiding this comment

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

LGTM, modulo a couple questions

paddingTop: CONTAINED_LEVEL_PADDING,
paddingLeft: CONTAINED_LEVEL_PADDING,
paddingRight: CONTAINED_LEVEL_PADDING,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: might be more extensible to put these into a containedLevelStyles object, a la the craftStyles

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -1791,6 +1791,10 @@ a.download-video {
box-sizing: border-box;
}

.contained-level .free-response .response {
resize: vertical;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this here and not in an apps stylesheet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mainly because this is where the rest of the free response level styling is.

Copy link
Contributor

Choose a reason for hiding this comment

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

Man, our CSS needs some normalization. This is a fine place, in that case.

</div>
{!this.props.collapsed && !this.props.isEmbedView &&
<HeightResizer
position={this.props.height}
onResize={this.handleHeightResize}
style={styles.heightResizer}
Copy link
Contributor

Choose a reason for hiding this comment

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

HeightResizer doesn't have a style property; what is this intended to do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added support for it in this PR. Is it bad practice to add a style property to react components?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh! Wow, I totally missed that. Nope, what you did is perfect.

@balderdash balderdash merged commit 1df1527 into staging Mar 6, 2017
@balderdash balderdash deleted the contained-level-scroll branch March 14, 2017 00:24
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.

None yet

2 participants