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

[AC-4742] Allow Multiple Selection When Editing in ListView #8761

Conversation

@Brianggalvez
Copy link
Contributor

commented Jan 18, 2017

  • Added AllowMultipleSelectionWhenEditing property
  • Added getSelectedRows method
  • updated docs
- Added getSelectedRows method
- updated docs
@hansemannn

This comment has been minimized.

Copy link
Contributor

commented Jan 18, 2017

Hey @Brianggalvez, before we start the review process: Did you create a JIRA ticket with some more background and a test-case so far? That's required in order to accept the PR. Thx!

Please check our Contribution Guidelines which are displayed above every new PR for more info 🚀

@Brianggalvez

This comment has been minimized.

Copy link
Contributor Author

commented Jan 18, 2017

Hi @hansemannn it could be part of this ticket
should I fill a new one with some test?

@hansemannn

This comment has been minimized.

Copy link
Contributor

commented Jan 18, 2017

From what I see, your PR adds the new getter for receiving the selected cells, the ticket describes native methods to be exposed to control this for the whole table view once. Better to have an own ticket (and Android parity) here.

@Brianggalvez Brianggalvez changed the title Allow Multiple Selection When Editing in ListView [AC-4742] Allow Multiple Selection When Editing in ListView Jan 18, 2017
@Brianggalvez

This comment has been minimized.

Copy link
Contributor Author

commented Jan 18, 2017

Ok, let's see now. I created a ticket to support just the features this PR adds. I've edited the PR note because it was missing the property addition.

Not sure if Android needs a parity ticket for this. Will investigate.

Copy link
Contributor

left a comment

First bunch of code-review finished, left some comments. Will continue when those are addressed, thx!

@@ -14,9 +14,9 @@ description: |
</table>
Use the <Titanium.UI.createListView> method or **`<ListView>`** Alloy element to create a `ListView`.

This comment has been minimized.

Copy link
@hansemannn

hansemannn Jan 18, 2017

Contributor

Please check your Atom settings, the whole PR has those blank lines in it, making it unreadable for reviews. Thx!

@@ -711,6 +711,13 @@ properties:
since: "5.4.0"
platforms: [iphone, ipad]

- name: AllowsMultipleSelectionDuringEditing

This comment has been minimized.

Copy link
@hansemannn

hansemannn Jan 18, 2017

Contributor

allowsMultipleSelectionDuringEditing

- name: AllowsMultipleSelectionDuringEditing
summary: Determines whether more than one row can be selected while editing the table.
type: Boolean
default: false

This comment has been minimized.

Copy link
@hansemannn

hansemannn Jan 18, 2017

Contributor

Did you implement the getter? Otherwise, this will return undefined and should be default: undefined (behaves as false).

summary: Determines whether more than one row can be selected while editing the table.
type: Boolean
default: false
since: "6.1.0"

This comment has been minimized.

Copy link
@hansemannn

hansemannn Jan 18, 2017

Contributor

6.2.0 for now.

parameters:
- name: markerProps
summary: Dictionary to describe the reference item.
type: ListViewMarkerProps
platforms: [iphone, ipad, android]
since: 4.1.0

- name: getSelectedRows

This comment has been minimized.

Copy link
@hansemannn

hansemannn Jan 18, 2017

Contributor

Ok. You should move this as a property, because the getters are generated automatically. Example:

    - name: selectedRows
      summary: returns the selected rows when AllowsMultipleSelectionDuringEditing is set to true.
      description: Returns an array of selected <Titanium.UI.ListItem> cells.
      platforms: [iphone, ipad]
      since: "6.2.0"
@@ -825,6 +825,14 @@ -(void)setDisableBounce_:(id)value
{
[[self tableView] setBounces:![TiUtils boolValue:value def:NO]];
}
-(void)setAllowsMultipleSelectionDuringEditing_:(id)args

This comment has been minimized.

Copy link
@hansemannn

hansemannn Jan 18, 2017

Contributor

Use:

- (void)setAllowsMultipleSelectionDuringEditing_:(id)value
 {
    ENSURE_TYPE(value, NSNumber);

    [[self tableView] beginUpdates];
    [[self tableView] setAllowsMultipleSelectionDuringEditing:[TiUtils boolValue:value]];
    [[self tableView] endUpdates];

    [[self proxy] replaceValue:value forKey:@"allowsMultipleSelectionDuringEditing_" notification:NO];
 }
  • are you sure you need the beginUpdates wrapper here? It's only required for some API's.
@@ -498,7 +498,39 @@ -(void)setContentInsets:(id)args
[self.listView setContentInsets_:arg1 withObject:arg2];
}, [NSThread isMainThread]);
}
- (NSMutableArray *)getSelectedRows:(id)args

This comment has been minimized.

Copy link
@hansemannn

hansemannn Jan 18, 2017

Contributor

- (NSMutableArray *)getSelectedRows:(id)unused

@@ -498,7 +498,39 @@ -(void)setContentInsets:(id)args
[self.listView setContentInsets_:arg1 withObject:arg2];
}, [NSThread isMainThread]);
}
- (NSMutableArray *)getSelectedRows:(id)args
{
NSMutableArray *result=[[NSMutableArray alloc]init];

This comment has been minimized.

Copy link
@hansemannn

hansemannn Jan 18, 2017

Contributor

I think it's ENSURE_ARG_COUNT(unused, 0) -> Ensure that there are no args provided.

NSMutableArray *result=[[NSMutableArray alloc]init];
NSArray *selectedRows = [self.listView.tableView indexPathsForSelectedRows];

if(selectedRows!=nil){

This comment has been minimized.

Copy link
@hansemannn

hansemannn Jan 18, 2017

Contributor

Indentation: if (selectedRows != nil) {

if(selectedRows!=nil){
TiThreadPerformOnMainThread(^{

for (NSIndexPath* indexPath in [self.listView.tableView indexPathsForSelectedRows]){

This comment has been minimized.

Copy link
@hansemannn

hansemannn Jan 18, 2017

Contributor

Indentation: ) {

brian garcía added 2 commits Jan 19, 2017
@hansemannn

This comment has been minimized.

Copy link
Contributor

commented Jan 23, 2017

Sorry @Brianggalvez, there are still 500+ blank lines changed making this PR unusable. Please do a fresh one based on changes being done either in Xcode or after configuring Atom to not use Soft Tabs.

@hansemannn hansemannn closed this Jan 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.