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

Make Row be agnostic of its presentation #60

Merged
merged 4 commits into from
Nov 18, 2015

Conversation

soffes
Copy link
Contributor

@soffes soffes commented Nov 14, 2015

Row shouldn't know anything about how its displayed (with the exception of specifying a class to display it). This was the original design of Static. I'm proposing going back to that mindset with a few small changes.

Remove Height

If you want a custom height for a cell, set tableView.estimatedRowHeight and then add a constraint to cell.contentView to set its height. This is a much more flexible way to go about custom heights if you need varying heights in the same table view.

Remove Image Tint Color

From the very beginning, you couldn't set a label's color. This was by design. If you wanted to change the presentation of a label, you had to make a custom cell. I think this same thinking applies to images. If you want to set the color of an image, either do it before it gets to the Row or do it in the cell. Having the Row tell the cell what color it is violates its lack of understanding of presentation.


I tried to explain my reasoning. Hopefully this doesn't seem silly. It's awesome to see people contributing to Static, but I think these two additions are going the wrong direction. Thoughts? ❤️

If you want a custom height for a cell, set tableView.estimatedRowHeight and then add a constraint to cell.contentView to set its height.
If you want to set the color of something, you should use a custom cell.
@dasmer
Copy link
Contributor

dasmer commented Nov 16, 2015

This reasoning makes sense to me. This would be a breaking API change though, so it might be worth either a) deprecating or b) version bumping

🍏

@eliperkins
Copy link
Contributor

Yeah, I definitely agree with you here. I think one thing this library could benefit from is showing users The Right Way™ to do these kinds of things in the README, so they're less inclined to submit PRs that change the libraries functionality and more inclined to just do it the way the README does.

I think I can whip a couple examples up and I'll PR them.

@soffes
Copy link
Contributor Author

soffes commented Nov 17, 2015

@dasmer Personally, I'd rather remove them and just bump the major version.
@eliperkins Sounds good :)

@ayanonagon
Copy link
Contributor

I’m happy with this revert as long as the tests get updated to reflect the change!

@soffes
Copy link
Contributor Author

soffes commented Nov 17, 2015

@ayanonagon done!

@soffes
Copy link
Contributor Author

soffes commented Nov 18, 2015

Cool if I click the green button?

hyperspacemark added a commit that referenced this pull request Nov 18, 2015
Make Row be agnostic of its presentation
@hyperspacemark hyperspacemark merged commit 9067f76 into venmo:master Nov 18, 2015
@soffes soffes mentioned this pull request Jan 1, 2016
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.

5 participants