Rich Text Editor: Scrollable and Custom Sizable Bottom Sheet #7085
Conversation
Velin92
commented
Nov 16, 2022
•
Velin92
commented
Codecov ReportBase: 11.74% // Head: 11.73% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## develop #7085 +/- ##
===========================================
- Coverage 11.74% 11.73% -0.02%
===========================================
Files 1620 1620
Lines 158933 159168 +235
Branches 64621 64723 +102
===========================================
+ Hits 18666 18673 +7
- Misses 139630 139858 +228
Partials 637 637
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
| @@ -25,11 +25,23 @@ class VectorHostingBottomSheetPreferences { | |||
| case medium | |||
| case large | |||
|
|
|||
| /// only available on iOS16, medium behaviour will be used instead | |||
| case custom(height: CGFloat, identifier: String = "custom") | |||
There was a problem hiding this comment.
| case custom(height: CGFloat, identifier: String = "custom") | |
| case custom(height: () -> CGFloat?, identifier: String?) |
It could be handy leveraging optional height to disable detents when using this API.
Also the identifier should be optional to match Apple's api.
Probably you also may need to make optional the return value of func uiSheetDetentId() but it shouldn't be a problem because it's used where optionals are expected.
There was a problem hiding this comment.
The thing is that we use detents only if we use the VectorBottomSheetPreferences inside the vectorHosting controller which could be nil in case we don't want to use the bottom sheet detents at all, which take in the constructor a non nill array of detents that are all assumed to be working... honestly adding these extra optional return value... seems an unnecessary overkill.
Essentially if we are using them, we probably want them to work. If we want to disable them, we just don't use the VectorBottomSheetPreferences at all (or we pass them with an empty array of supported Detents)
There was a problem hiding this comment.
I understand your reasoning but in my opinion doing so you still remove flexibility in our api.
Changing the API like I suggest you have dynamic detents behaviors like this:
let detents: [VectorHostingBottomSheetPreferences.Detent] = [
.medium,
. custom(
height: {
guard someCondition else {
return nil
}
return someComputationReturningCGFloat()
}, identifier: nil)
]And I think the effort should be quite small.
There was a problem hiding this comment.
dynamic detents would require to also use the function invalidateDetents, because custom detents are only recalculated if such function is called as explained in the docs here:
https://developer.apple.com/documentation/uikit/uisheetpresentationcontroller/detent/3976719-custom
In fact if the calculation depends on a specific value that is stored and then this value changes, nothing would happen until the detents are invalidated (since their size depends on a closure)
It's more effort than what it seems to have dynamic custom detents, and I don't think is worth the effort for this level of flexibility, since it would impact the overall readibility of the wrapper, or just to fix for now one single screen
There was a problem hiding this comment.
I was thinking mainly when you start presenting a new vc.
In that case you don't need to invalidate since the presentation didn't start yet.
Btw you could do the same with your proposed implementation returning different array of detents.
So IGTM ;-)
|
Kudos, SonarCloud Quality Gate passed!
|
