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

Themes: Hide no search results text when showing themes #9783

Conversation

@mchowning
Copy link
Contributor

commented May 3, 2019

Overview

This PR prevents the "No themes matching your search" text from being visible in the gaps between the items in the list of themes in ThemeBrowserFragment.

output

Background

There are 3 overlapping views from ThemeBrowserFragment that are relevant to this PR:

New Name Previous Name Description
mNoThemesView mEmptyView Informs the user that no themes have been retrieved (either due to a fetch currently in progress or no internet connection)
mNoMatchesView mActionableEmptyView Informs the user that there are "No themes matching your search"
mGridView [no change] Displays the list of (possibly filtered) themes to the user

Before this PR, there were only two states for visibility of these views:

View No Themes Downloaded Visibility Themes Downloaded Visibility
mNoThemesView VISIBLE GONE
mNoMatchesView GONE VISIBLE
mGridView GONE VISIBLE

Because mGridView and mNoMatchesView overlap each other, and mGridView is higher on the z-axis, any themes that are displayed will (mostly) obscure mNoMatchesView's "No themes matching your search" text. If the user performs a search doesn't match any of the themes (so no themes are displayed), then the mNoMatchesView is uncovered, and it becomes fully visible to the user.

But the mNoMatchesView is visible beneath the list of themes even when themes are being displayed. Therefore, the "No themes matching your search" text can be seen in the gap between the top two visible themes. It is most noticeable in landscape:

output_landscape1

Changes in this PR

This PR changes the handling of these views so that instead of two possible states (themes-not-downloaded and themes-downloaded), there are now three states, with only one view visible in each state:

State Visible View
No themes have been downloaded mNoThemesView
Themes have been downloaded, and at least one theme is displayed mGridView
Themes have been downloaded, and no themes are displayed due to a user search mNoMatchesView

Before this PR, we were not updating the visibility of these views when the filter changed. That is why both mNoMatchesView and mGridView had to be visible any time we had downloaded themes—we were not updating the views based on whether any of those downloaded themes were being displayed. This PR detects any changes to the adapter's data set (including changes caused by the user's search) by adding a data set observer to the adapter. Any time the adapter's data set changes, including when the user's search changes, that observer is triggered. We then check which of the three possible states we are in, and update the views accordingly.

Test Steps

  1. Log into a fresh install of the app (or any install of the app that has not already downloaded the themes)
  2. Tap on 'Themes' from the 'My Site' tab
  3. Observe the screen display "Fetching themes..." below an image of a Drake while the themes are being downloaded.
  4. Once the themes are downloaded, observe that the list of themes is displayed.
  5. Scroll the list up and down so that the gap between the themes crosses the entire screen and confirm that neither the (a) "Fetching themes..."; or (b) "No themes matching your search" text is visible in the gaps between the themes.
  6. Execute a search that filters all of the themes (i.e., 'asd')
  7. Observe the "No themes matching your search" text is visible
  8. Shorten the previous search so that some themes are displayed again (i.e., 'as')
  9. Again confirm that there are no views visible underneath the list of themes.

Update release notes

  • If there are user facing changes, I have added an item to RELEASE-NOTES.txt.
@peril-wordpress-mobile

This comment has been minimized.

Copy link

commented May 3, 2019

Warnings
⚠️ PR is missing at least one label.

Generated by 🚫 dangerJS

@mchowning mchowning changed the title Fix/themes hide themes no search results Themes: Hide no search results text when showing themes May 3, 2019

@nbradbury

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

Just a friendly reminder to please make sure to manually merge develop into this branch before you merge this PR, as develop now contains the AndroidX migration. The automatic merge can succeed but still break the build. Better to find out now before it breaks develop. Thanks!

@elibud elibud requested a review from khaykov Jun 6, 2019

@elibud elibud added this to the 12.7 milestone Jun 6, 2019

@khaykov khaykov self-assigned this Jun 7, 2019

mchowning added some commits May 2, 2019

Hide no-search-results-view when displaying themes
Previously, the theme view would display either (a) the
fetching/no-internet-connection view, or (b) both the list of themes
and (underneath that) a view saying "No themes matching
your search" (which is visible in the gaps between list items). This
fixes that by only showing the "No themes matching your search" view
when all the available themes are being filtered out by the search.

@mchowning mchowning force-pushed the mchowning:fix/themes_hide_themes_no_search_results branch from 5841acc to 7fede78 Jun 7, 2019

@mchowning

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

please make sure to manually merge develop into this branch before you merge this PR

Thanks for the heads up @nbradbury ! I went ahead and just rebased my changes onto develop. That seemed like the simplest way to avoid any issues.

@khaykov
Copy link
Member

left a comment

Thanks for the PR, @mchowning ! Fix works as expected 👍 I just left a couple of minor comments about naming :)

boolean hasVisibleThemes = getAdapter().getCount() > 0;
boolean hasNoMatchingThemes = hasThemes && !hasVisibleThemes;

mNoThemesView.setVisibility(!hasThemes ? RelativeLayout.VISIBLE : RelativeLayout.GONE);

This comment has been minimized.

Copy link
@khaykov

khaykov Jun 7, 2019

Member

I know that the original code used RelativeLayout.VISIBLE/GONE, but do you mind switching them to View.GONE/VISIBLE for consistency with the rest of codebase?

This comment has been minimized.

Copy link
@mchowning

mchowning Jun 11, 2019

Author Contributor

Updated to import the View constants instead of the RelativeLayout constants.

@@ -8,7 +8,7 @@
android:layout_height="match_parent">

<org.wordpress.android.ui.ActionableEmptyView
android:id="@+id/actionable_empty_view"
android:id="@+id/no_matches_view"

This comment has been minimized.

Copy link
@khaykov

khaykov Jun 7, 2019

Member

I know that no_matches_view and no_themes_view/no_themes_text below makes more sense in the context of themes fragment, but customary (and for convenience) we are mostly using generic names for empty views, like actionable_empty_view (since we use them a lot, and it makes it easy to reference them all around the place) :)

This comment has been minimized.

Copy link
@mchowning

mchowning Jun 11, 2019

Author Contributor

Sure thing. I have reverted the name changes.

makes it easy to reference them all around the place

Would be curious to know a bit more about this. Obviously, my inclination was to go the other way and to use names specific to this fragment. I'd really like to learn more about the considerations that led us to go the other direction. 😀

This comment has been minimized.

Copy link
@khaykov

khaykov Jun 12, 2019

Member

I wish I could give you a good answer on why we are doing it, but I don't have one :D Actionable empty views were added to the app relatively recently, and we added them with the same ID for reasons no one probably remembers :)

@mchowning

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

Thanks for the review @khaykov ! I believe I've addressed your comments and this is ready for a re-review.

@khaykov
Copy link
Member

left a comment

Thanks for the changes! Looks good :shipit:

@khaykov khaykov merged commit 186fd77 into wordpress-mobile:develop Jun 12, 2019

4 checks passed

Peril :warning: Danger found some issues. Don't worry, everything is fixable.
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: strings-check Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details

@mchowning mchowning deleted the mchowning:fix/themes_hide_themes_no_search_results branch Jun 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.