New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature request: Support for menu to display triggered by button tap #827

Closed
rudygalfi opened this Issue Nov 1, 2015 · 36 comments

Comments

@rudygalfi
Copy link
Contributor

rudygalfi commented Nov 1, 2015

Menus can help drive circulation. This request covers the ability to show a menu once the user has clicked/tapped on a button presented on the page. The most common patterns for presenting a menu seem to be:

  • Upon pressing the menu button, the menu overlays the content on the page. There's some way to get rid of the menu either by clicking/tapping another button/link (likely in the same place as the original button) or by clicking/tapping outside the menu area.
  • Once the button is pressed, the menu slides in from the side and displaces the content (it slides as well). You can get rid of the menu either by clicking some button in the newly exposed menu area or by clicking in the content area (if some of it is still visible on-screen).

@rudygalfi rudygalfi added the REQUEST label Nov 1, 2015

@dvoytenko

This comment has been minimized.

Copy link
Collaborator

dvoytenko commented Nov 1, 2015

Rudy, wouldn't this be a viewer's function?

@rudygalfi

This comment has been minimized.

Copy link
Contributor

rudygalfi commented Nov 2, 2015

@dvoytenko I could see this behavior in the viewer, but that still raises the question of how that would work. Would there be some way of being able to define the menu content and, upon seeing that, a viewer provide a button that exposes the menu?

What I'm trying to reflect with this issue is some feedback I've heard from some publishers around enabling menus in some way. If you look at some mobile sites like http://mobile.nytimes.com/ or http://www.theguardian.com/mobile you can see examples, and that's the kind of pattern I have in mind here. These menus enable navigation to other areas like the Business or Sports section.

@dvoytenko

This comment has been minimized.

Copy link
Collaborator

dvoytenko commented Nov 2, 2015

One approach would be to leave it completely up to the viewer implementation which is currently responsible for the header itself. That could work well in the publisher sites (e.g. Publisher.com viewer would show the Publisher.com's menu), but the question will be: should the menu be shown when the same story is displayed by, e.g. Google Search or by Twitter or others. The issue here might be that those apps may have their own menus.

@cramforce

This comment has been minimized.

Copy link
Member

cramforce commented Nov 2, 2015

There isn't always a viewer.
The most basic request here is to change layout based on whether there is.

I don't really understand the feature request, though. Whatever we do with menus must be responsive, so it does something reasonable on phone, tablets and desktop.

@rudygalfi

This comment has been minimized.

Copy link
Contributor

rudygalfi commented Nov 2, 2015

Let me know how I can clarify things. One thing I can say is that in creating this request I had the standalone (i.e. non-viewer) case in mind, so you should read everything that way: assuming no viewer. (I agree that it's interesting to consider menus in the context of a viewer as well.) I also agree that whatever we have should be responsive.

The current behavior we see is that publishers are providing links to other areas of their site (Sports section, Business section) through a statically defined list that's included in the page. You can't have any interactivity, so there's no way to press a button to be able to see the menu. This request captures some component that would allow this.

@mjsarfatti

This comment has been minimized.

Copy link

mjsarfatti commented Nov 18, 2015

+1
I think the classic hamburger menu is seen more often than carousels or lightboxes, even though I see difficulties in finding one common way of implementing it (variations on the structure of the menu list are infinite, how would you manage submenus and sub-submenus?)

@maxhartshorn

This comment has been minimized.

Copy link

maxhartshorn commented Dec 16, 2015

Yes this would be a very useful feature to have. I expect a user reading an article from any news site would want to have some way of navigating to other sections from the page they're in.

@heatloss

This comment has been minimized.

Copy link

heatloss commented Dec 29, 2015

I would highly urge that the HTML5 [details] and [summary] tags be supported, as these would allow for dynamic menus and accordion widgets without the need for any custom JavaScript.

@stephanfowler

This comment has been minimized.

Copy link

stephanfowler commented Jan 19, 2016

Menus on our regular site are geo-dependent. In the context of a single cached AMP article, this means the menu would have to be loaded following an interaction, which could make a call to our site and receive a geo-varied response.

So our requirement for menus is: an element (eg a "burger"), that when clicked, unhides a container and executes any AMP markup within it (such as amp-list etc.)

That only needs something like an Expando component, rather than anything specific to menus. Menus themselves are going to be too hard to generalise, imho.

@samthurman

This comment has been minimized.

Copy link

samthurman commented Feb 4, 2016

+1
We show share button menus with an initial tap, this feature would allow us to do that. Very much wanted!

@dkolba

This comment has been minimized.

Copy link

dkolba commented Feb 6, 2016

What's the ETA of Milestone M2?

@rudygalfi

This comment has been minimized.

Copy link
Contributor

rudygalfi commented Feb 9, 2016

@dkolba Roughly March/April. We have a lot in M2 currently, though, so some stuff may get further pushed out.

We're starting to make progress on menu-like functionality currently, however. cc @ericlindley-g

@steffenweber

This comment has been minimized.

Copy link
Contributor

steffenweber commented Feb 11, 2016

Just a few thoughts: In HTML5 there is a hidden attribute that can be applied to any element. AMP should toggle (add/remove) this attribute to hide/show content.

Hidden state:

<button aria-controls="mypanel">Toggle</button>
<div id="mypanel" hidden>Text Text Text ...</div>

Expanded state:

<button aria-controls="mypanel" aria-expanded="true">Toggle</button>
<div id="mypanel">Text Text Text ...</div>
@gandmexpress

This comment has been minimized.

Copy link

gandmexpress commented Feb 17, 2016

Hi there, I'm brand new here and to AMP-html... actually I just founded today... but I just can't understand (after hours of looking into amp-html) why there would be no menu/nav options.. can someone please shed some light on this? So is really just to build single webpages... what's the purpose if you can't jump to a new page or have a navigation menu to help discover more of the owners site? Thank you

@Nemo64

This comment has been minimized.

Copy link
Contributor

Nemo64 commented Feb 18, 2016

@gandmexpress I'm not an amp dev but i think the reason is simple. Amp is still in its imfancy. A month ago it wasn't even possible to include analytics on a page.

@gandmexpress

This comment has been minimized.

Copy link

gandmexpress commented Feb 18, 2016

@Nemo64 makes sense... thank you.

@rudygalfi

This comment has been minimized.

Copy link
Contributor

rudygalfi commented Feb 18, 2016

Yes, as @Nemo64 said, AMP is still quite young. We've come a long way and we've built features in priority order based on feedback from publishers and content providers. (We also appreciate contributions to starting with an intent to implement and design overview to help solve the problems you're most interested in.) Menus are on our near-term roadmap. Another factor playing into the prioritization decision is that:
(a) you can build nav, just not necessarily with all of the interactive features you may like; and
(b) we've seen creative attempts to build menus using existing components like AMP lightbox, and while we think a more proper solution is needed, folks have made progress that way.

@gandmexpress

This comment has been minimized.

Copy link

gandmexpress commented Feb 18, 2016

@rudygalfi Rudy, thank you. I appreciate you taking the time to reply. I will look into alternatives mentioned in the mean time. I really like the idea/concept of what AMP is and will continue to bring the internet (specifically mobile). Thx again

@jpettitt

This comment has been minimized.

Copy link
Collaborator

jpettitt commented Mar 2, 2016

You can achieve the same results (constrained to one open section) with :focus CSS selectors. I stole borrowed the idea from the Washington Post, they are doing 'hamburger' menus that way.

See https://www.washingtonpost.com/amphtml/politics/clinton-trump-victories-foreshadow-nasty-contentious-fall-campaign/2016/03/02/106c6dd6-e0ae-11e5-8d98-4b3d9215ade1_story.html

It's a bit tricky to get right and you need to put a tabindex on the elements or it won't work and you may need to add :active if you don't want things to disappear when links are clicked. Overall it seems to work pretty well. We're using it for footer menus and hamburger menus.

CSS

#accordion .footer-menu {
    display: none;
}
#accordion .heading:focus ~ .footer-menu {
    display: block;
}

html

<div id="accordion">
        <section class="panel">
            <a tabindex="99999" class="heading">Stuff</a>
            <ul class=" footer-menu">
                <li><a href="/">A link</a></li>
                <li><a href="/">a.n.other link</a></li>
            </ul>
        </section>

        <section class="panel">
            <a tabindex="99999" class="heading">Really interesting stuff</a>
            <ul class=" footer-menu">
                <li><a href="/">Customer Service</a></li>
                <li><a href="/">About Us</a></li>
            </ul>
        </section>

        <section class="panel">
            <a tabindex="99999" class="heading">Social, Mobile &amp; More</a>
            <ul class=" footer-menu">
                <li><a href="/">Facebook</a></li>
                <li><a href="/">Twitter</a></li>
                <li><a href="/">Mobile</a></li>
            </ul>
        </section>
</div>
@Nemo64

This comment has been minimized.

Copy link
Contributor

Nemo64 commented Mar 3, 2016

@jpettitt a nice idea. If we want workarounds I might as well just add the :target selector. It would not only allow for a collapsing menu but would also allow to use the back button on android to close it again. And it is also possible to scroll within the menu while still allowing other hash links to close it again. And it might work pretty well with accessibility when done right (put menu at the end of the page?).

I tried to construct something: https://jsfiddle.net/d6qr7qsh/1/

@rudygalfi rudygalfi modified the milestones: Backlog, M2 Mar 4, 2016

@dvoytenko

This comment has been minimized.

Copy link
Collaborator

dvoytenko commented Mar 4, 2016

@jpettitt :focus most likely works with AMP JS on iOS Safari because we set cursor:pointer on html.

@steffenweber @Nemo64 :target is a very interesting idea. Although I'm leaning more and more toward a amp-sidebar or amp-menu element with flexible styling - there are just too many little bugs when working with position:fixed, scrolling and all the rest.

@steffenweber

This comment has been minimized.

Copy link
Contributor

steffenweber commented Mar 4, 2016

Making :target work would have the nice side-effect of a free fallback for page requests with disabled JavaScript or JS errors.

@dvoytenko

This comment has been minimized.

Copy link
Collaborator

dvoytenko commented Mar 5, 2016

@steffenweber @Nemo64 There's definitely a way to fix :target issue - #2456 is in review. That does have a nicer way to trigger the menu. However, that would leave with with fewer options to cancel the menu, e.g. by clicking outside or swiping. I think some JS intervention will be unavoidable, but I agree, the default behavior is of :target is good.

@rudygalfi

This comment has been minimized.

Copy link
Contributor

rudygalfi commented Mar 8, 2016

For folks interested in this issue, there's a pending pull request for an amp-sidebar component: #2461. Please take a look.

/cc @sriramkrish85 @ericlindley-g

@rudygalfi

This comment has been minimized.

Copy link
Contributor

rudygalfi commented Mar 30, 2016

Is #2461 tracking to land by tomorrow's release, or should we shift this into the next sprint?

@rudygalfi

This comment has been minimized.

Copy link
Contributor

rudygalfi commented Apr 1, 2016

Rolling this into next milestone.

@camelburrito

This comment has been minimized.

Copy link
Collaborator

camelburrito commented Apr 10, 2016

Update:

#2461 - has been split into smaller PRs and all of them have landed.

#2788 , #2792, #2795 , #2811, #2812, #2813, #2823

There are a few more kinks that i am working on (#2859 - is one of them!)

Feel free to experiment and file bugs!

@camelburrito

This comment has been minimized.

Copy link
Collaborator

camelburrito commented Apr 13, 2016

@ericlindley-g This is ready for experimentation! and should be fully working now. There are a few validation fixes that needs to be done, then this should be fully functional (after a lot of testing!)

@ericlindley-g

This comment has been minimized.

Copy link
Collaborator

ericlindley-g commented Apr 13, 2016

Excellent! Is the plan to push to this week's canary?

/cc @edlea-g to take a look at the implementation

@camelburrito

This comment has been minimized.

Copy link
Collaborator

camelburrito commented Apr 13, 2016

@ericlindley-g yes (will still be an experiment)

@rudygalfi

This comment has been minimized.

Copy link
Contributor

rudygalfi commented Apr 14, 2016

Given there's additional open PRs (#827 (comment)) and assuming this is the meta-bug covering all of amp-sidebar, should we roll this over into the next milestone?

@camelburrito

This comment has been minimized.

Copy link
Collaborator

camelburrito commented Apr 14, 2016

@rudygalfi - all the PRs have been merged now. Sidebar is ready for experimenting (as mentioned above)

@rudygalfi

This comment has been minimized.

Copy link
Contributor

rudygalfi commented Apr 14, 2016

@sriramkrish85 Thanks, I'll close this out then.

@rudygalfi rudygalfi closed this Apr 14, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment