Skip to content
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

Added delete callback for RowType instances #1136

Conversation

AdamStreet
Copy link

@SkylerSeamans
Copy link

This would be incredibly helpful! Hope it gets a review soon.

@Raymond-em
Copy link

This is will be very useful, also a similar callback to reordering of rows

@mtnbarreto
Copy link
Member

mtnbarreto commented Jul 13, 2017

Hi @AdamStreet!
I'm not sure that we need this callback to get notifications about multivalued rows deletion.

I would prefer a more semantic name and also expose the property from another class, probably MultivaluedSection so we make it clear that the closure only works for multivalued sections. With the current implementation, it seems that the willBeRemoved is called each time a row is removed from the form which is wrong.

An alternative solution could be:

The Section class conforms to SectionDelegate so implements the following methods:

/// The delegate of the Eureka sections.
public protocol SectionDelegate: class {
    func rowsHaveBeenAdded(_ rows: [BaseRow], at: IndexSet)
    func rowsHaveBeenRemoved(_ rows: [BaseRow], at: IndexSet)
    func rowsHaveBeenReplaced(oldRows: [BaseRow], newRows: [BaseRow], at: IndexSet)
}

So you can extend MultivaluedSection to override these methods (do not forget to call super implementation).

Anyway I'm gonna move the FormDelegate methods from extension to FromViewController class to make them extensible.

@AdamStreet
Copy link
Author

Hi @mtnbarreto,

Thank you for reviewing my request.
The reason why I chose to add this callback - even the naming is not optimal and also works on Rows of Section instances - instead of overriding the delegate method(s) was the -onChange method syntax. If you handle the value change event in an expression it was just as convenient to do so when the row was or will be removed.

Can you please describe a scenario when the current implementation causes "wrong" behaviour?

With the current implementation, it seems that the willBeRemoved is called each time a row is removed from the form which is wrong.

If the user removes a row by hitting the delete button it fires the callback - I haven't tested if it was done programatically but I would expect the same when that happens.

I haven't found any issue using this implementation but I am happy to switch if it is unreliable or has a hidden flaw inside.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants