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

More fine-grained control over data in ListComponent #21

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

zachbryant
Copy link

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.

@redwarp
Copy link
Contributor

redwarp commented Jul 31, 2019

@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

@redwarp redwarp requested a review from dwaxemberg July 31, 2019 13:52
@dwaxemberg
Copy link
Member

bump

@zachbryant
Copy link
Author

Thanks for the reminder! Things are currently crazy at school but I'll have time to revise my request after Halloween

@redwarp
Copy link
Contributor

redwarp commented Feb 5, 2020

@zachbryant bump?

@zachbryant
Copy link
Author

zachbryant commented Feb 12, 2020

@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

@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 index seems to be in line with appendData too, unless I'm missing something? Let me know and I'll change it to an adjusted index

Copy link
Contributor

@redwarp redwarp left a 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
Copy link
Contributor

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
Copy link
Contributor

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) {
Copy link
Contributor

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

Comment on lines +102 to +110
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")
Copy link
Contributor

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
Copy link
Contributor

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);
Copy link
Contributor

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

@redwarp
Copy link
Contributor

redwarp commented May 27, 2021

@zachbryant is this still alive?

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.

3 participants