xwp / travel

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

Footer template #64

Closed miina closed 6 years ago

miina commented 6 years ago

Fixes #44 and #58.

Adds one menu location footer-menu which is displayed in the footer. Also makes the copyright year dynamic and fixes the home link.

Screenshot with partly added menu: screen shot 2018-04-23 at 1 52 04 pm

miina commented 6 years ago

@kienstra This should also be ready for review, however, just realized that according to the AC there should be two menu locations instead of one. The approach in the PR uses just one menu location where the "heading" of the menu is a parent menu and the clickable menu items is a submenu.

Do you think it would be better to rework the PR to have two separate menu locations? Thoughts?

kienstra commented 6 years ago

Menu Locations Hi @miina,

Do you think it would be better to rework the PR to have two separate menu locations? Thoughts?

No, I think the 'footer-menu' on this PR looks good:

dynamic-footer-menu

I think creating the menu for that is intuitive:

footer-menu
kienstra commented 6 years ago

Question About Styling Of Footer

Hi @miina, Could you please help with the styling of the footer? It looks like on archive.php pages, the footer styling isn't appearing.

I can see some of the footer styling on this line in homepage.css, like with the selector .travel-footer. But I'm not sure exactly how much of that line might need to be in the archive.css file.

Feel free to ask Mike about this, if you'd like. Thanks!

miina commented 6 years ago

@kienstra Thanks for reviewing.

I think it's better if @mehigh takes a look at the design, tried taking the CSS from here: https://github.com/ampproject/ampstart/blob/master/css/templates/travel/components/_footer.css and replacing the variables with the data from here: https://github.com/ampproject/ampstart/blob/master/css/templates/travel/page-vars.css but it didn't work as expected.

Note: Looks like according to Maxim's designs, the search.php also has the footer, added it to search.php template, too.

@mehigh Do you think you would have time to take a look at the CSS of footer.php for search.css and archive.css?

mehigh commented 6 years ago

I've recompiled the CSS in my local amp-start and the footer shows up well. I noticed the missing footer search, so I've added that in as well.

Phone: https://dsh.re/dac98 Laptop: https://dsh.re/b59c1

kienstra commented 6 years ago

Thanks, Footer Styling Looks Good on search.php Question About Other Issues

Hi @mehigh, Thanks, the footer styling looks good on the search.php page. But the images don't seem to display:

images-search-results

Also, the footer styling doesn't look to be present on the archive.php pages, like /location/australia/:

archive-styling

Also, it looks like the <input> and <button> from the email form don't display on search.php:

search-input-button
kienstra commented 6 years ago

Update: Last Screenshot Above Is Invalid

Hi @mehigh, Sorry, the last screenshot in my comment above is inaccurate. The email form didn't appear because I didn't build the menu correctly.

It look like the only issue there is that the "Discover" section should appear to the right of the "Company" section.

The styling for that looks good on the homepage.

search-php-menu
mehigh commented 6 years ago

I've updated the patch to make the footer navs work OK. It turns out one patch was applied to homepage and product, but never to the search. Instead I got rid of it and added some different classes on the menu to not need extra CSS for this behaviour.

mehigh commented 6 years ago

@kienstra Thanks to your suggestion for fixing the images. I also added the footer search to archive and the footer styling as well.

kienstra commented 6 years ago

Latest Changes Approved

Hi @mehigh, Thanks a lot for your improvements here. The footer looks great on the homepage, archive.php, search.php, and single 'adventure' page.

kienstra commented 6 years ago

Building Menu

Please see these screenshots to build the menu, and please use footer-menu as the location.