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

Adding --depth option to border command to recursively border subviews #126

Merged
merged 3 commits into from
Nov 6, 2015

Conversation

eithanshavit
Copy link
Contributor

No description provided.

@kastiglione
Copy link
Contributor

Interesting idea, thanks! We'll get this reviewed soon.

layer = viewHelpers.convertToLayer(view)
setBorder(layer, options.width, color, colorClassName)
else:
assert depth <= 0, "Recursive bordering is only supported for UIViews"
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you want to have two separate asserts? One on the class, and another on depth?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you please elaborate a bit, or give an example? Notice that I've changed the code a bit in a new commit, but assert still in place.

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like it should be something more like:

assert true, "Recursive bordering is only supported for UIViews"
assert depth <= 0, "Recursive bordering requires a depth greater than 0"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There might be a misunderstanding here. The purpose of this assert is to make sure recursive bordering is not used for layers (or anything that's not a UIView or NSView). However regular bordering is still permitted for layers. So if you're in the code path that takes you to this else block, we should validate that depth is not provided to make sure recursive bordering won't happen.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. It reads like it's assert bad values on the depth, yet giving an unrelated error message.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. It reads like it asserts bad values on the depth, yet giving an unrelated error message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll add a comment that should make it clearer.

@kastiglione
Copy link
Contributor

Thanks @eithanshavit 💚

@kastiglione kastiglione closed this Nov 6, 2015
@kastiglione kastiglione reopened this Nov 6, 2015
@kastiglione
Copy link
Contributor

Oops, wrong button.

kastiglione added a commit that referenced this pull request Nov 6, 2015
Adding --depth option to border command to recursively border subviews
@kastiglione kastiglione merged commit fc2f2cf into facebook:master Nov 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants