romaonthego / RETableViewManager

Powerful data driven content manager for UITableView.
MIT License
2.48k stars 432 forks source link

Inconsistent margins on devices wider than iPhone 4 #259

Open jlnr opened 8 years ago

jlnr commented 8 years ago

We have noticed inconsistent left and right margins on iPhone 6+ and iPad, and tracked them down to this spot:

https://github.com/romaonthego/RETableViewManager/blob/1c71a41a09df510399d258b36cac52e014bd7685/RETableViewManager/RETableViewCell.m#L171-L177

I guess this was written under the assumption that the default left margin for table cells is always 10.0, which is only true on the smaller iPhone models. We have worked around this issue in our fork by changing the method like this:

https://github.com/jlnr/RETableViewManager/commit/90fb0b39d94d94784d4c4ec5f41c192ce1864d50

I guess one could also fix the inconsistency by resizing self.textLabel, which iOS provides by default, to match the size and margins of RETableViewManager’s custom views. But we want our app to look as native as possible.

This is not ready for a Pull Request because the cellOffset of 10.0 also appears in other cell types which we don't use and haven't tested, and I don't know if we have broken the way the contentViewMargin works.

If you compare the before/after screenshots of the example on iPhone 6+ and iPad, it really looks much more consistent. This is possibly related to issues #249 and #115.

iPhone 6 Plus, before

iphone6plus-before

iPhone 6 Plus, after

iphone6plus-after

iPad, before

ipad-before

iPad, after

ipad-after

jlnr commented 8 years ago

The safest way to fix this would be to make use of different margins in the example app, so that we can visually verify that things look ok.

XBeg9 commented 8 years ago

@jlnr what about self.tableView.cellLayoutMarginsFollowReadableWidth ?

XBeg9 commented 8 years ago

@jlnr my idea to move all modules (views) to submodules, so you will be able to add them as subspec. For example I don't use any integrated views and use only the manager itself. I will talk with @romaonthego about that.

jlnr commented 8 years ago

We don't want to disable cellLayoutMarginsFollowReadableWidth, because we want to blend in with the iOS defaults. However, even setting it to NO only reduces the offsets, it doesn't completely fix them. Just tested in in the iPad simulator on iOS 9.3.

jlnr commented 8 years ago

Modularising the pod won't hurt, but the individual parts should still look consistent afterwards. So I think that's an unrelated issue actually :)

XBeg9 commented 8 years ago

@jlnr a bit related :) moving all into modules, we don't need to fix all custom views in core. I will dig into this issue a bit later. I think this can be fixed easily with subclass.