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

Fixes click on RecyclerView when ItemView is wrapped #48

Merged
merged 6 commits into from
May 3, 2017

Conversation

Sloy
Copy link
Member

@Sloy Sloy commented May 3, 2017

This PR fixes the issue in #47.

On PR #40 we introduced a change to avoid clicking wrong things in a RecyclerView row, and allow clicking child views inside a row.

Unfortunately, that last change also broke the click when the ClickListener is set in a child view and not directly in the ItemView, as described in #47.

In this PR we:

  • Reproduce the issue with a broken test (WrappedViewClickTest).
  • Fix some flaky tests on RecyclerViewTest that were already giving false positives.
  • Fix the original issue by only using performClick() when the view is clickable, or else use Espresso's click() action to allow propagating the click as usual.

Sloy added 4 commits May 3, 2017 16:23
This test proves that click() and clickUsingPerformClick() have different behaviors when clicking the parent (wrapper) of a view.
Tests are passing even though the feature is broken. \o/
The click listener should be on the row (itemView), not in the label (textView).
Otherwise clicking the row won't open the next activity.
Otherwise use espresso's click action (which simulates a tap)
@@ -21,32 +21,32 @@
@Test
public void checkClickRecyclerViewItem_byPosition_atTwo() {
clickRecyclerViewItem(R.id.recycler, 2);
assertDisplayed("Avocado");
assertDisplayed("Avocado has been clicked");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks :·)

view.performClick();
} else {
click().perform(uiController, view);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this is a bit of magic, we should explain why are we doing this trick. I don't have any suggestions but I feel that project newcomers will cry when they see this thing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe if we extract the line click().perform(uiController, view); to a method called propagateClickToChildren()?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, YES! Great name!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! 🎉

new ActivityTestRule<>(WrappedViewActivity.class);

@Test
public void click_on_button() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are tons of tests written with the camel case style. If you prefer to start using other style, please, change all Barista's tests.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shit. Old habits.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! 🎉

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Party! 😄

@rocboronat
Copy link
Member

It's merge time! Feel free to bump a new version 🎉

@Sloy Sloy merged commit 87147e3 into master May 3, 2017
@Sloy Sloy deleted the feature/fix-click-on-wrapper branch May 3, 2017 15:35
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.

2 participants