xwp / travel

WordPress AMP Adventures Themes (Native AMP)
GNU General Public License v3.0
34 stars 6 forks source link

Search results page #45

Closed miina closed 6 years ago

miina commented 6 years ago

This PR is based on #42. Fixes #43.

Adds search results page with dynamic data using amp live list.

Todo:

screen shot 2018-04-18 at 8 33 34 pm

miina commented 6 years ago

@mehigh Finished the template for now, the search CSS can be found from assets/css/search.css, likely that there is something that needs fixing. Also, there is no search button, should we create this (cc @kienstra)? Or how should the search work?

kienstra commented 6 years ago

Proposed Search Button

Hi @postphotos, Could you please help with @miina's question about the search button?

Also, there is no search button, should we create this (cc @kienstra)? Or how should the search work?

The design doesn't look to have a search button on the search results page:

no-search-button

And neither does the AMP Start theme on Firebase. What do you think of this possible "search" button that I created?

possible-search-button

The button is a few pixels too tall, though.

<button class="ampstart-btn rounded bold">Find Adventures</button>

Styling (though this wouldn't be added inline in the actual PR):

element.style {
    background: rgba(0,0,0,.2);
    margin-left: 25px;
}
kienstra commented 6 years ago

This applies to Issue #29.

miina commented 6 years ago

@kienstra This should also be ready for review now, added the search button, too. Note that on the mobile view the inputs and filters are removed and tried to make something similar to the static theme's view. One more note -- currently there is no design / template for no results page, added just a text. Could this stay like that? Or should we create something better?

@mehigh Would be great if you could also review the template, specifically for HTML / CSS adjustments.

kienstra commented 6 years ago

Question About "No Results"

Hi @postphotos, Could you please help with @miina's question:

One more note -- currently there is no design / template for no results page, added just a text. Could this stay like that? Or should we create something better?

Here's how that looks at the moment:

no-results
kienstra commented 6 years ago

Request For Help With Styling

Hi @mehigh, Are you available to help with some styling points here and here? Thanks for all of your work on this theme.

miina commented 6 years ago

Hey @kienstra, thanks again for reviewing, the issues that are not related to styling should be fixed now.

kienstra commented 6 years ago

Thanks, PR Approved Other Than Styling

Hi @miina, Thanks for addressing those points. This is approved, other than the styling points here and here.

mehigh commented 6 years ago

I've identified the reason the images look squashed. In the demo all of the images had a width of 2 and a height of 1, which corresponded to a 2:1 format image. Since the images in your screenshot were not 2:1 that didn't work. I'm switching this to use the correct aspect ratio by reading the width & height from the thumbnail meta.

mehigh commented 6 years ago

The reason the "$" wasn't appearing alone in the demo theme https://www.ampstart.com/templates/travel/travel-results.amp was the fact that after the span containing the $ there was no whitespace or newline. The fix for this was simply removing the whitespace altogether. There was correctly a non-breakable space in there, it only needed to be alone in order for the effect to work correctly.

mehigh commented 6 years ago

@kienstra this should be good to go now, as i've handled all 3 styling issues reported - price wrapping on a separate line than the $, the grid and the image formats.

kienstra commented 6 years ago

Thanks!

Hi @mehigh, Thanks a lot for fixing the display of the images, adding a flex wrapper, and making the $ always appear next to the price. This looks good.

kienstra commented 6 years ago

New Image Size To Account For 640px Display

If it's alright, I added a new image size of 600x300. When the viewport is 640px, the image is about 600px:

image-roughly-600-300

Before, the images had a lower resolution at that size. This keeps the expected 2-1 ratio.

In testing this, it helped me to use the WP-CLI comand:

wp media regenerate