adds new checkmark placeholder #125
Conversation
| @@ -23,6 +23,10 @@ public struct Row: Hashable, Equatable { | |||
| /// Checkmark | |||
| case checkmark | |||
|
|
|||
| /// Checkmark Placeholder. | |||
| /// Allows spacing to continue to work when switching back & forth between checked states. | |||
| case checkmarkPlaceholder | |||
There was a problem hiding this comment.
Is this case necessary given:
https://github.com/venmo/Static/pull/125/files#diff-083b9db3d36d23083693e650c2604474R37
Allows for any view to be set?
There was a problem hiding this comment.
I thought that we are probably running into this problem elsewhere in the app, and by resolving it here, we could solve it once for many places.
There was a problem hiding this comment.
Given:
- disclosureIndicator
- checkmark
- switch
- customView
Each having different sizes
Would we also want a placeholder for these as well should we not want text to fill the space in cells we were using those items as well?
There was a problem hiding this comment.
if those are problematic for layout, we could certainly add those.
right now, I'm just trying to solve for the checkmark
There was a problem hiding this comment.
The goal of Static is:
...to separate model data from presentation.
Rows andSections are your “view models” for your cells.
If the Row is the view model, does a value used solely for presentation belong there? To be sure, presentation has already crept into Row, but we are adding to that with this addition.
I do agree that the convenience provided by this code is something that Static looks to provide. But I would like to see two things addressed:
- The magic number 24 (can we get that value from the image—although I know that it is nigh impossible)
- Providing placeholders for all cases as @dmiluski suggests, as I believe Static needs to be a complete solution for all use cases
There was a problem hiding this comment.
I think the point of this PR is not to reimagine what Static is or is not, but to provide an easy way for a user to mark a cell for the potential to have a checkmark, so the rest of the content view is laid out appropriately.
24 was the result of testing this across various sized iPhones. I don't know of a way to predict this otherwise.
I don't have the time right now to add placeholders for every potential scenario – if that's a requirement for Static, I can kill this PR, and do a one-off solution in the venmo codebase.
thanks y'all
|
What do you think of handling this with a one-off |
|
@eliperkins I think this'll be useful for others who might want their content to not move around when showing a check or not showing a check. |
We were running into an issue where if you toggled back & forth between checked/unchecked states, the content would abruptly shift back & forth for the user, on smaller screens like the 5s. (The problem didn't exhibit itself on larger screens like the X).
This allows the developer to opt-in to use a
checkmarkPlaceholder, allowing Static to insert a placeholder appropriately sized blankUIViewto hang out until a checkmark may appear in its place.