-
Notifications
You must be signed in to change notification settings - Fork 33
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
More fine-grained control over data in ListComponent #21
base: master
Are you sure you want to change the base?
More fine-grained control over data in ListComponent #21
Conversation
@zachbryant that would be a great addition! It seems though that this change does not take into account the presence or not of dividers. If the ListComponent has separator, then it also displays some grey line between each element. In effet, it displays some extra items, which will impact the index of the item being added. Check https://github.com/Yelp/bento/blob/master/bento/src/main/java/com/yelp/android/bento/components/ListComponent.java#L105 |
bento/src/main/java/com/yelp/android/bento/components/ListComponent.java
Outdated
Show resolved
Hide resolved
bento/src/main/java/com/yelp/android/bento/components/ListComponent.java
Outdated
Show resolved
Hide resolved
bump |
Thanks for the reminder! Things are currently crazy at school but I'll have time to revise my request after Halloween |
@zachbryant bump? |
…p component to demonstrate/test methods
@redwarp I noticed that no matter what index is given, as long as the item count is correct the whole list re-renders. Not sure if that's intended.. but passing just |
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.
Please add tests to ListComponentTest to verify that proper index are updated with and without dividers.
@@ -1,5 +1,6 @@ | |||
.* | |||
*.iml | |||
.idea |
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 don't think this should be there
@@ -1,6 +1,7 @@ | |||
package com.yelp.android.bentosampleapp | |||
|
|||
import android.os.Bundle | |||
import android.util.Log |
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 not be there, Log is not used
@@ -79,8 +81,7 @@ class ListViewActivity : AppCompatActivity() { | |||
controller.addComponent(simpleComponent) | |||
} | |||
|
|||
|
|||
private fun addListComponent(controller: ComponentController) { | |||
private fun addListComponent(controller: ComponentController, removable: Boolean = false) { |
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.
the removable flag is not used
listOf(1, 1, 1, 2, 4).forEachIndexed { index, i -> insertData(i, "List element ${index + 2}") } | ||
insertData(0, "List element First") | ||
insertData(0, "List element First") | ||
insertData(1 + component.count / 2, "List element Last") | ||
removeData("List element First") | ||
val index = removeData("List element 1") | ||
insertData(index, "List element 1") | ||
removeData("List element 2") | ||
removeData("List element nonexistent") |
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 don't think this is a really useful code: you are adding and removing items even before the component would be displayed on screen. You might want to consider instead first displaying a list of items, and adding a button somewhere to try the scenario where you remove items dynamically.
@@ -1,6 +1,7 @@ | |||
package com.yelp.android.bentosampleapp |
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.
Be aware that the ListViewActivity was created to verify that list views display stuff properly. There is no link between the ListViewActivity and the ListComponent. Bento is compatible with both ListViews and RecyclerViews, ViewPagers...
public void insertData(int index, @NonNull T data) { | ||
mData.add(index, data); | ||
// Update 2 items if dividers are showing. | ||
notifyItemRangeInserted(index, mShouldShowDivider ? 2 : 1); |
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.
if divider are visible, you probably need to multiply index by 2, otherwise you will have some surprises
@zachbryant is this still alive? |
This commit improves the flexibility of ListComponent by enabling more fine-grained updates to the data. The return type of ListComponent.removeData(T data) now returns the int value of the index removed. Additionally a new method to insert data at a certain index in the existing data has been added. The user should be able to replace or re-insert data into the list without re-rendering the entire list to change one item.