Skip to content

Reader search design review - #4122

Merged
maxme merged 151 commits into
developfrom
feature/reader-search-master
Jun 22, 2016
Merged

Reader search design review#4122
maxme merged 151 commits into
developfrom
feature/reader-search-master

Conversation

@nbradbury

Copy link
Copy Markdown
Contributor

Reader search passed code review in PR #4096, now it just needs a design review before it can be merged into develop.

Needs review @jancavan

nbradbury and others added 30 commits May 4, 2016 21:10
…ss-Android into feature/reader-search-master

Conflicts:
	WordPress/src/main/java/org/wordpress/android/datasets/ReaderDatabase.java
	WordPress/src/main/java/org/wordpress/android/datasets/ReaderTagTable.java
@nbradbury

Copy link
Copy Markdown
Contributor Author

Thanks for the design review, @jancavan!

For search history, might be good to also be able to clear one item

Added in aa4a6e3

Perhaps it should say "Clear search history"?

Added in c5d2664

Search bar text can be made smaller and lighter and should probably say "Search WordPress.com" instead of "Search on WordPress.com"

The text size and color is determined by Android itself. With some effort we can customize it, but I think it's better to stick with the defaults here. Good call on using "Search WordPress.com" - added in 99d5868

When there is more than a single line of text, the top/bottom padding are inconsistent. A top/bottom padding of around 16px would be good.

Yuck, glad you noticed that! I think it's unlikely that users will enter such a long search query, though. What do you think about simply limiting dropdown items to a single line, like this?

single-line

For the "Clear saved search" dialog box, we can update: the text to say: Clear recent searches (heading), Would you like to clear your recent searches? (subtext) have a division between the heading and subtext no/yes button changed to blue (blue_wordpress / #0087be) instead of orange

The dialog style is determined by Android itself and is consistent with other dialogs in the app, but I agree the text could use some work. However, the text "Would you like to clear your recent searches?" is a bit long for mobile, and goes against Google's recommendations here.

screen shot 2016-06-04 at 8 45 20 pm

Looking at Google's recs for dialogs, I think we can drop the dialog title entirely and simply use this:

dialog

I added that in de89715. Thoughts?

@nbradbury

nbradbury commented Jun 5, 2016

Copy link
Copy Markdown
Contributor Author

"No results" should probably match the web version's styling:

@jancavan Those colors are used throughout the app for similar messages, so I think they shouldn't change.

That text copy I'm using for no results matches what we currently have on Calypso search, but I'm happy to change it. However, I don't think it should be changed to use "We couldn't find any..." since Google recommends avoiding the pronoun "We".

we

I did tweak it slightly and added <em> formatting of the query string in bf08137 - let me know if you'd prefer different copy.

device-2016-06-05-070808

@jancavan

jancavan commented Jun 7, 2016

Copy link
Copy Markdown

Thanks for working on the updates, it's looking good!

The text size and color is determined by Android itself. With some effort we can customize it, but I think it's better to stick with the defaults here.

Gotcha. Ok, let's just keep the default styles then.

What do you think about simply limiting dropdown items to a single line, like this?

Good call, I think this is great. When I checked the branch out, there's a space between the last letter and ellipsis. Or is the ellipses added, I think, after 30 characters including spaces?

Looking at Google's recs for dialogs, I think we can drop the dialog title entirely and simply use this

Yup, this works. Much better than the original.

That text copy I'm using for no results matches what we currently have on Calypso search, but I'm happy to change it. However, I don't think it should be changed to use "We couldn't find any..." since Google recommends avoiding the pronoun "We".

Ok, makes sense. I think the latest version is good.

Re aa4a6e3, sorry I missed this the last time, but I think it's worth adding a dialog box (Would you like to clear the recent search alligator?) to avoid inadvertently deleting a recent search item.

Re the Search icon, I also noticed the white Search icon isn't a Gridicon - so both white and gray one:

screenshot 2016-06-07 16 01 53

screenshot 2016-06-07 16 01 58

@nbradbury

Copy link
Copy Markdown
Contributor Author

I think it's worth adding a dialog box (Would you like to clear the recent search alligator?) to avoid inadvertently deleting a recent search item

@jancavan I think that may be overkill, since all the user is doing is removing a query string (no other data is affected). I do agree with a confirmation when clearing all items, though.

Re the Search icon, I also noticed the white Search icon isn't a Gridicon - so both white and gray one

Sorry - I missed that in your previous comment. Can you provide SVGs?

@jancavan

jancavan commented Jun 9, 2016

Copy link
Copy Markdown

Here's the Search icon SVG. Let me know if there's anything else you need.

https://cloudup.com/c9ffqkTBmCv

@jancavan

jancavan commented Jun 9, 2016

Copy link
Copy Markdown

Design LGTM 👍

@nbradbury

Copy link
Copy Markdown
Contributor Author

@maxme This PR is ready to be merged now that it has passed design review, but I wasn't sure if we needed to wait until the support lib bug you mentioned here is addressed?

@maxme

maxme commented Jun 10, 2016

Copy link
Copy Markdown
Contributor

@maxme This PR is ready to be merged now that it has passed design review, but I wasn't sure if we needed to wait until the support lib bug you mentioned here is addressed?

Support library v24.0 should be out soon, we could at least wait 2 weeks (before wpandroid 5.6 freeze) before merging this.

@nbradbury

nbradbury commented Jun 15, 2016

Copy link
Copy Markdown
Contributor Author

Update: Support library v24.0.0 is out and apparently fixes the SearchView crash. Submitted #4236 so we can give the new support lib a spin.

@maxme

maxme commented Jun 22, 2016

Copy link
Copy Markdown
Contributor

Support library v24.0.0 was merged, so it's time to :shipit:!

@maxme
maxme merged commit 23fed49 into develop Jun 22, 2016
@maxme
maxme deleted the feature/reader-search-master branch June 22, 2016 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants