New search UI #834
New search UI #834
Conversation
ghost
assigned 
There was a problem hiding this comment.
Lookin good @joeyg ! Just a couple changes I'm noticing:
- Can we make the title "Firefox Lockbox" be in the standard page title styling (as before), instead of using the Large Title?
- I saw your note about not being able to use opacity to define the search box background. Can you then update the background to be this color: #11608A (17, 96, 138)
Nice work on this, looks great from the autofill credential list also!
|
Finally fixed the background color on the search bar. However that resulted in a loss of space between the search icon and search text |
| XCTAssertEqual(noMatches, 1) | ||
|
|
||
| // Tap on cacel | ||
| app.buttons["cancelFilterTextEntry.button"].tap() | ||
| let searchFieldValueAfterCancel = searchTextField.value as! String | ||
| app.buttons["Cancel"].tap() |
There was a problem hiding this comment.
ideally we will continue to use the accessibilityLabels here if possible
| @@ -52,6 +52,15 @@ extension UIImage { | |||
| draw(in: CGRect(origin: .zero, size: size)) | |||
| return UIGraphicsGetImageFromCurrentImageContext() | |||
| } | |||
|
|
|||
| static func color(_ color: UIColor, size: CGSize=CGSize(width: 1, height: 1)) -> UIImage? { | |||
| UINavigationBar.appearance().isTranslucent = false | ||
| } | ||
|
|
||
| UINavigationBar.appearance().barTintColor = Constant.color.navBackgroundColor |
| self.navigationItem.largeTitleDisplayMode = .always | ||
|
|
||
| let searchController = UISearchController(searchResultsController: nil) | ||
| searchController.obscuresBackgroundDuringPresentation = false |
There was a problem hiding this comment.
this might be better abstracted into a func styleSearchController(sc: UISearchController) -> UISearchController so that these methods are easier to read
|
According to @joeyg on Dec 30 (sorry I missed it) this is ready for another UX review. |
There was a problem hiding this comment.
@joeyg I'm still seeing the large title for Firefox Lockbox when I first sign in. If I force close the app, then re-open, the title is in the normal style. However, if I go to Settings and then return to the entry list, it is the Large Title again. Similar behavior happens when I Search and then cancel, as well as navigating to an entry detail and back.
If we're having to hack this to get it so that the title appears in the normal style vs. the large style with this approach, and it's taking a fair amount of time, I'd say we just leave it with the Large Title always showing. It's not a huge deal, as I mentioned before it was just a nice to have to keep that navigation bar a bit more condensed. Thanks for your work in trying to resolve this.
One other small thing I'm noticing:
When I pull to refresh, sometimes the Title and Search portion don't "snap" back up to the top, leaving a good amount of empty space (see screenshot)
There was a problem hiding this comment.
Tested out on iPhone X and works pretty nicely!
One thing I noticed is the "tap to top" (of the screen) takes me up to the top and the large title appears, but so does a chunk of "pull to refresh" spinner and stays there..
..is that easily catchable/preventable? Related to what @nickbrandt noticed above? Or worth filing as a separate bug to try and squash later?
And now that I've used it a bunch, I like seeing our app name nice and big when I open it any way. ✨ 🙃
|
@devinreams let me try the fix mentioned here to resolve the scroll to top issue |
There was a problem hiding this comment.
One thing I noticed is the "tap to top" (of the screen) takes me up to the top and the large title appears, but so does a chunk of "pull to refresh" spinner and stays there..
Thanks for that last fix, @joeyg. It's scrolling back up and working smoothly for me now. 🏅
When I pull to refresh, sometimes the Title and Search portion don't "snap" back up to the top, leaving a good amount of empty space (see screenshot)
@nickbrandt with that latest change/build, I wasn't able to experience and reproduce this. Are you able to anymore?
Are we good to merge this with your final approval? 👍 👎
|
@joeyg @devinreams looks good! No longer experiencing the pull to refresh issue |
|
Thanks @nickbrandt @joeyg, I'm excited to get this over to TestFlight so our internal testers and beta folks can start hammering on it. |
Fixes #427
Blocked by #822 landing.