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

Layout improvements #128

Merged
merged 16 commits into from
Jan 20, 2016
Merged

Layout improvements #128

merged 16 commits into from
Jan 20, 2016

Conversation

erikschlegel
Copy link
Contributor

Creating on behalf of Jason.

Review on Reviewable

@erikschlegel
Copy link
Contributor Author

Reviewed 1 of 10 files at r1.
Review status: 1 of 10 files reviewed at latest revision, 4 unresolved discussions.


ReactWindows/ReactNative/UIManager/BorderedContentControl.cs, line 30 [r1] (raw file):
What are your thoughts on abstracting the border re-sizing logic into a separate class? Perhaps with a follow-on push so we're able to get the functionality out the door.


ReactWindows/ReactNative/UIManager/BorderedContentControl.cs, line 32 [r1] (raw file):
Can you add some more comments to explain the rationale behind the calculations below.


ReactWindows/ReactNative/UIManager/BorderedContentControl.cs, line 36 [r1] (raw file):
Might be helpful to create a GeometryPointClass to track the lower, upper and center point calcs and track the 4 points in a collection. Perhaps the calculation block can be isolated into it's own function or class to help with unit testing all calc related scenarios.


ReactWindows/ReactNative/UIManager/DependencyProperties.cs, line 7 [r1] (raw file):
Can you document any public classes and methods.


Comments from the review on Reviewable.io

erikschlegel added a commit that referenced this pull request Jan 20, 2016
@erikschlegel erikschlegel merged commit d3d12ca into Hackfest Jan 20, 2016
@Foxman13 Foxman13 deleted the layoutImprovements branch January 21, 2016 17:45
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.

1 participant