-
Notifications
You must be signed in to change notification settings - Fork 281
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
Margin resize #95
Margin resize #95
Conversation
b3f707c
to
e140335
Compare
FittedSheetsPod/SheetSize.swift
Outdated
@@ -14,6 +14,7 @@ public enum SheetSize: Equatable { | |||
case fixed(CGFloat) | |||
case fullscreen | |||
case percent(Float) | |||
case margin(CGFloat) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be .fullscreenWithTopMargin(CGFloat)
, to indicate what the base size is. .margin(CGFloat)
doesn't clearly indicate what it will be a margin on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea here, just would like the name of the enum value changed
With having margin it will not be fullscreen anymore.
because of the fixed leading trailing and bottom just top lefts to have margin so in my opinion its ok but if you don’t like that how about .fixedWithMarginFromTop?
… On Aug 13, 2020, at 11:18 PM, Gordon Tucker ***@***.***> wrote:
@gordontucker commented on this pull request.
In FittedSheetsPod/SheetSize.swift <#95 (comment)>:
> @@ -14,6 +14,7 @@ public enum SheetSize: Equatable {
case fixed(CGFloat)
case fullscreen
case percent(Float)
+ case margin(CGFloat)
This should probably be .fullscreenWithTopMargin(CGFloat), to indicate what the base size is. .margin(CGFloat) doesn't clearly indicate what it will be a margin on.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub <#95 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFCZR6447FJ3U3MXJT7W2NDSAQYOZANCNFSM4P6U5TXQ>.
|
It isn't a fixed size, so that wouldn't work. How about just |
done |
We can define height by constant margin from top now.