themes icon indicating copy to clipboard operation
themes copied to clipboard

Published 20 hours ago • verified publisher icon Automattic

Coutoire: Include Menu Enhancements

Open Aurorum opened this issue 3 years ago • 8 comments

Changes proposed in this Pull Request:

  1. Ensures that the logo doesn't overlap with the text content

https://user-images.githubusercontent.com/43215253/121583945-d40c2380-ca28-11eb-8deb-1acda6a321d1.mov

  1. Allows users to ensure their menu doesn't exist. As mentioned in https://github.com/Automattic/themes/issues/1901#issuecomment-630361066, this will automatically be enabled for people who active the theme for the first time, but not for those who already have it active.
Screenshot 2021-06-10 at 20 18 27
  1. Adds some space around the footer links
Screenshot 2021-06-10 at 20 19 11

Related issue(s):

Fixes #1901 Fixes #1838 Fixes #2072 Fixes #4022

Aurorum avatar Jun 10 '21 19:06 Aurorum

If someone in design can take a look, that'd be rad. We get this request a fair bit and the usual answer has been "Sorry, change themes" even though they love this one otherwise. TIA!

supernovia avatar Sep 09 '21 18:09 supernovia

@kjellr

scruffian avatar Sep 10 '21 09:09 scruffian

  1. Ensures that the logo doesn't overlap with the text content

This implementation doesn't seem to be working well in my testing:

  • The "jump" that happens when you scroll is a little jarring. This could be smoothed out with animation.
  • When the logo becomes smaller, the space it originally took up remains large, which leads to a lot of unnecessary scrolling before you hit content. We could slide the content up to account for this when the logo becomes smaller.
  • If I scroll back up to the top of the page, the logo remains small and off to the left. Instead, I think it should return to its original size when you get back up there.

scroll

If we can smooth out those issues with some animation and polish, I think this is a viable change.

It is a little weird to make the logo so small but still to let the site title & tagline overlap the main page content. But I guess we'll have that checkbox to turn this off entirely.

  1. Allows users to ensure their menu doesn't exist.

This seems fine, but this option should live in Content Options, not Site Identity, since it's effecting more than just the site title.

  1. Adds some space around the footer links

This change seems fine.

kjellr avatar Sep 13 '21 11:09 kjellr

Thank you for the review! I agree with the issues which you've raised, and I'll address them this week.

Aurorum avatar Sep 15 '21 18:09 Aurorum

Hi! Those points should be addressed now, but please let me know if you run into any other issues. :)

https://user-images.githubusercontent.com/43215253/133885507-042daf2d-2478-4a01-92a9-b25ea554d0e5.mov

Aurorum avatar Sep 18 '21 10:09 Aurorum

This seems to work pretty well! I'm seeing just a couple small bugs:

  1. The scale-down animation doesn't seem to work the first time it's triggered (on the initial page load). It still jumps for me.
  2. When I scroll back up to the top, there's a little spacing jump that happens where the page content jumps down the page a little bit. Is there any way we can smooth that out?

styles

kjellr avatar Sep 28 '21 17:09 kjellr

Hi @kjellr, apologies for the delay. I've been trying to reproduce both these issues but I haven't been able to in Chrome 94 - could you confirm if you're using a different browser, as I wonder if there might be an issue there?

Aurorum avatar Oct 08 '21 17:10 Aurorum

I made some changes to the approach here: https://github.com/Automattic/themes/pull/4899

scruffian avatar Oct 25 '21 09:10 scruffian