-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Add support for nested global entries #651
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
base: master
Are you sure you want to change the base?
Conversation
|
In Swift, if you don't use the closure argument, like this: FLEXManager.shared.registerGlobalEntry(withName: "Name") { _ in
print("Log")
}you now get the build error: and the only workaround that I can think of is changing the call to: FLEXManager.shared.registerGlobalEntry(withName: "Name") { (_: UITableViewController) in
print("Log")
}or FLEXManager.shared.registerGlobalEntry(withName: "Name") { (_: FLEXUserGlobalEntriesContainer) in
print("Log")
}But this means that you have to look into FLEX's files to find this out, if you're new to it. So, I will rename the new method (from Swift's POV, the first parameter name), to avoid this ambiguity: FLEXManager.shared.registerNestedGlobalEntry(withName: "Name") { _ in
print("Log")
} |
|
@NSExceptional, do you have time to look over this? 😁 |
|
Not at the moment no; I was actually in a serious car accident on Tuesday and just got out of the hospital yesterday 🤕 I reviewed a PR yesterday but it was just a small bug fix |
|
I'll try to get to this when I can use my hand again |
|
OMG, I'm so sorry to hear that 😱 Forget about this PR, get well soon 🤗 |
544945a to
5c4a218
Compare
|
Hey dawg! I'm finally getting around to clearing out all these PRs and issues that have accumulated over the last year. Sorry for the delay. I appreciate your patience (and frequent contributions). Still reviewing, but I'm curious, what's the use case for changing the cell accessory type? |
This is so we can remove the (now) superfluous methods that only forward the same parameters to the new container object.
5c4a218 to
94b6883
Compare
|
@NSExceptional, would you have some time to check this, and hopefully merge it? 🙏🏻 It would reduce the overhead of using and syncing a fork in order to use this functionality. |
| FLEXGlobalsViewController *controller = [FLEXGlobalsViewController new]; | ||
| controller.customEntries = FLEXManager.sharedManager.globalEntriesContainer.entries; | ||
| controller.showsDefaultEntries = YES; | ||
|
|
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.
Instead of doing this little dance each time we create one of these, can we just add a convenience init that does this for us so that it becomes [FLEXGlobalsViewController newWithCustomEntries] or something? It can even have that BOOL parameter if you think that's useful
|
|
||
| + (instancetype)entryWithEntry:(Class<FLEXGlobalsEntry>)entry row:(FLEXGlobalsRow)row; | ||
| + (instancetype)entryWithEntry:(Class<FLEXGlobalsEntry>)entry | ||
| cellAccessoryType:(UITableViewCellAccessoryType)cellAccessoryType | ||
| row:(FLEXGlobalsRow)row; | ||
|
|
||
| + (instancetype)entryWithNameFuture:(FLEXGlobalsEntryNameFuture)nameFuture | ||
| cellAccessoryType:(UITableViewCellAccessoryType)cellAccessoryType | ||
| viewControllerFuture:(FLEXGlobalsEntryViewControllerFuture)viewControllerFuture; | ||
|
|
||
| + (instancetype)entryWithNameFuture:(FLEXGlobalsEntryNameFuture)nameFuture | ||
| cellAccessoryType:(UITableViewCellAccessoryType)cellAccessoryType | ||
| action:(FLEXGlobalsEntryRowAction)rowSelectedAction; |
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.
In addition to these new ones, can we keep the original signatures around and have them default to the disclosure indicator? Removing them is a breaking change I'd like to avoid
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 probably need to just set up a pre-commit linter hook, but in the meantime can we discard all the trailing whitespace changes?
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.
A lot of the comments on these methods are repeated for each method. Can we just describe the future blocks and their caveats (i.e. the @note about their lifetimes) in a doc comment on the type itself? It would shorten this file dramatically.
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.
Also, can we rename this to Custom instead of User?
| /// Removes all registered global entries. | ||
| - (void)clearGlobalEntries; |
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.
Is this method necessary at all? I'm having a hard time imagining a scenario in which you'd want to un-register everything, or anything for that matter
| /// @note The passed block will be copied and retain for the duration of the application, | ||
| /// you may want to use __weak references. | ||
| - (void)registerGlobalEntryWithName:(NSString *)entryName objectFutureBlock:(id (^)(void))objectFutureBlock; | ||
| - (void)registerGlobalEntryWithName:(NSString *)entryName objectFutureBlock:(id (^)(void))objectFutureBlock __attribute((deprecated("Use the same method on the globalEntriesContainer property instead."))); |
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 see no reason to depreciate the old method, I'm not in a hurry to add more warnings for people to deal with
| - (void)registerGlobalEntryWithName:(NSString *)entryName objectFutureBlock:(id (^)(void))objectFutureBlock __attribute((deprecated("Use the same method on the globalEntriesContainer property instead."))); | |
| - (void)registerGlobalEntryWithName:(NSString *)entryName objectFutureBlock:(id (^)(void))objectFutureBlock; |
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.
And so on for the rest below
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.
Does this really need to be its own type? Any reason it can't be an extension on FLEXManager itself?



Please review each commit separately, as this PR is logically split into multiple "slow" transitions from the current implementation to the new one.