Toggling classes on click #1032

cramforce · 2015-12-01T01:11:24Z

I believe we discussed this before, but I think it might the time to do this.

<button on="tap:element.toggleClass(someClass)">

or similar.

I'd like to avoid this being used to toggle ads, so I was thinking to limit it on elements that do not contain ads. One could still e.g. change zIndexes to put ads behind content. Anyone have an idea how to make it more foolproof?

CC @dvoytenko @rudygalfi

rudygalfi · 2015-12-01T01:36:50Z

@cramforce Do you think this is the same as #1017?

AliberGzz · 2016-01-12T15:48:47Z

This will be possible ?

sebastianbenz · 2016-02-25T18:45:17Z

Have we got a timeline for this? Would love to have a show/hide boilerplate button for http://amp-by-example.appspot.com.

dvoytenko · 2016-02-25T19:46:40Z

We got according now that could be used for this, I think. /cc @sriramkrish85

camelburrito · 2016-02-25T19:58:35Z

@sebastianbenz <amp-accordion> should let you do that.
amp-accordion is now experimental - we will need to add validator support for this.

sebastianbenz · 2016-02-26T09:41:54Z

@sriramkrish85 @dvoytenko Works splendid! When is it going to be released?

sebastianbenz · 2016-02-26T14:46:09Z

@sriramkrish85 @dvoytenko FYI https://amp-by-example.appspot.com/amp-accordion.html

dvoytenko · 2016-02-26T19:39:59Z

@sebastianbenz I'll leave it to @rudygalfi to state the dates.

rudygalfi · 2016-02-27T00:12:08Z

We need to get the validation in. @ericlindley-g is going to track that down with @Gregable.

rudygalfi · 2017-02-18T00:31:17Z

@ericlindley-g @choumx To resolve if this is covered by bind, could be folded into bind, or should be closed.

ericlindley-g · 2017-02-18T00:33:41Z

Yep—covered by bind. Closing for now, but folks should feel free to re-open if there's a need for a simpler syntax, as proposed above.

westonruter · 2018-02-02T21:42:52Z

@ericlindley-g I think more is needed here. In particular, what if there are multiple variable classes on an element already (such as those added to body by WordPress), and we need to toggle just one of them? Currently this can't be done with bind because isn't able to mutate the existing classes.

If bind were able to reference the existing state that could be a solution. So maybe something like this, with WP template code to show intended usage:

<body <?php body_class(); ?> [class]="ampConf.mobileMenu ? add( 'no-scroll' ) : remove( 'no-scroll' )'" >

It would be important for amp-bind to compare the class attribute in a way that ordering of the class names doesn't matter, or else we'd get:

amp-bind: Default value for [class] does not match first expression result (). This can result in unexpected behavior after the next state change.

So perhaps it is better to go the action route as initially proposed.

<body id="body" <?php body_class(); ?>>
<button on="tap:body.toggleClass('no-scroll')">Toggle menu</button>

There should probably be addClass and removeClass as well.

ericlindley-g · 2018-02-02T21:51:50Z

Good point, @westonruter — reopening and /cc @choumx for visibility

dreamofabear · 2018-02-03T04:29:50Z

A bit clunky, but you could store the default classes in an <amp-state> component:

<body [class]="toggleVariable 
    ? defaultBodyClasses.concat('no-scroll') 
    : defaultBodyClasses">

But yea, this sounds a lot like #10622. And @sebastianbenz's suggestion in particular: #10622 (comment)

toggleClass() action makes sense for simple cases, but won't interoperate with amp-bind so we could build both options. We do have show() and hide() actions which also overlap with amp-bind's functionality. This might make a decent GFI actually.

westonruter · 2018-05-03T04:39:09Z

Here's how I ended up implementing a state-based toggle on the body element in WordPress:

<body <?php body_class(); ?> [class]="navMenu.expanded ? <?php echo esc_attr( wp_json_encode( get_body_class( 'sidebar-open' ) ) ) ?> : <?php echo esc_attr( wp_json_encode( get_body_class() ) ); ?>">

Importantly, this only would allow for toggling a single class. It's also not very pretty.

westonruter · 2018-05-03T04:53:20Z

In the WordPress theme there is a button that invokes jQuery to toggleClass:

<button class="menu-toggle" aria-controls="primary-menu" aria-expanded="false"><?php esc_html_e( 'Primary Menu', '_s' ); ?></button>

~~Here's how I worked out to accomplish (mostly) the same with AMP, with hacking a label to work like a button that toggles a hidden checkbox:~~ (Update: This is wrong. See #1032 (comment))

<amp-state id="siteNavigationMenu">
	<script type="application/json">
		{
			"expanded": false
		}
	</script>
</amp-state>
<input
	id="site-navigation-expanded"
	type="checkbox"
	hidden
	on="change:AMP.setState( { siteNavigationMenu: { expanded: event.checked } } )"
>
<label
	class="menu-toggle"
	for="site-navigation-expanded"
	tabindex="0"
	role="button"
	aria-controls="primary-menu"
	aria-expanded="false"
	[aria-expanded]="siteNavigationMenu.expanded ? 'true' : 'false'"
>
	<?php esc_html_e( 'Primary Menu', '_s' ); ?>
</label>

Notice importantly that the “button” isn't only needing to cause the body class to toggle. It also needs to cause the aria-expanded attribute to toggle. This would be well served by an AMP action which toggles some state. With such a state toggle, the above checkbox+label could be restored as a button, something like this: (Update: This is unnecessary. See #1032 (comment))

<button
	class="menu-toggle"
	aria-controls="primary-menu"
	aria-expanded="false"
	[aria-expanded]="siteNavigationMenu.expanded ? 'true' : 'false'"
	on="change:AMP.toggleState( 'siteNavigationMenu', 'expanded' )"
>
	<?php esc_html_e( 'Primary Menu', '_s' ); ?>
</button>

~~Where toggleState takes a list of args that corresponds to the state tree branch to toggle.~~

sebastianbenz · 2018-05-03T14:33:19Z

Interesting problem. Here is another approach that supports toggling multiple body classes:

<body class="a b c" [class]="globalState.join(' ')">
  
  <amp-state id="globalState"><script type="application/json">["a", "b", "c"]</script></amp-state>
  
  <amp-bind-macro id="toggle" 
                arguments="value" 
                expression="globalState.includes(value) ? globalState.filter(x => x != value) : globalState.concat(value)" />
  
  <button on="tap:AMP.setState({ globalState: toggle('a') })" aria-expanded="true" [aria-expanded]="[globalState.includes('a')]">toggle 'a'</button>
  <button on="tap:AMP.setState({ globalState: toggle('b') })" aria-expanded="true" [aria-expanded]="[globalState.includes('b')]">toggle 'b'</button>
  <button on="tap:AMP.setState({ globalState: toggle('c') })" aria-expanded="true" [aria-expanded]="[globalState.includes('c')]">toggle 'c'</button>

</body>

Full example: https://jsbin.com/qilalovife/edit?html,output.

It's still not very nice, but solves the problem. It'd be nicer if amp-bind-macro would support actions (or a new extension amp-bind-action.

westonruter · 2018-05-03T17:04:39Z

@sebastianbenz Good idea to use concat. That made me realize I could actually avoid manipulating the body class state entirely, in favor of dynamically concatenating a class name based on whether or not some other state value changes:

<body <?php body_class(); ?> [class]="minnow.bodyClasses.concat( minnow.navMenuExpanded ? 'sidebar-open' : '' ).filter( className =&gt; '' != className )">
<amp-state id="minnow">
	<?php
	$state = array(
		'bodyClasses'     => get_body_class(),
		'navMenuExpanded' => false,
	)
	?>
	<script type="application/json"><?php echo wp_json_encode( $state ); ?></script>
</amp-state>

Given this value of the [class] attribute, the .concat( minnow.navMenuExpanded ? 'sidebar-open' : '' ) part could be repeated any number of times according to the class names that should toggle based on changes to state props. Maybe it's simpler than your example, but maybe it's not as great either.

Note: The => here is due to a limitation in the AMP for WordPress plugin, noted in ampproject/amp-wp@52b3616

Aside: I found that if you return an array as the value of [class] it automatically gets concatenated with a space.

westonruter · 2018-05-03T17:54:08Z

What I wrote in #1032 (comment) was wrong. It is trivial to toggle state already using straight AMP.setState(). All that is needed is to add an on attribute like so:

<button
	class="menu-toggle"
	aria-controls="primary-menu"
	aria-expanded="false"
	on="tap:AMP.setState( { siteNavigationMenu: { expanded: ! siteNavigationMenu.expanded } } )"
	[aria-expanded]="siteNavigationMenu.expanded ? 'true' : 'false'"
>...

sebastianbenz · 2018-05-03T18:02:23Z

:-) even better. I thought being able to toggle multiple classes on the same element was your problem, which can result in awkward nested if statements.

ericlindley-g added this to the M2 milestone Feb 6, 2016

ericlindley-g added Type: Feature Request Related to: Content labels Feb 6, 2016

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

This was referenced Nov 21, 2016

Make checkbox-hack legal in amp html #6259

Closed

Add easy way to toggle a class based on user input #3501

Closed

adelinamart assigned rudygalfi Feb 3, 2017

adelinamart modified the milestones: New FRs, Backlog Bugs Feb 3, 2017

adelinamart removed the To be deprecated - Related to: Content label Feb 13, 2017

rudygalfi assigned ericlindley-g and dreamofabear and unassigned rudygalfi Feb 18, 2017

ericlindley-g closed this as completed Feb 18, 2017

westonruter mentioned this issue Feb 2, 2018

Issue 38: Rework header menu on mobile. xwp/ampnews#44

Merged

ericlindley-g reopened this Feb 2, 2018

dreamofabear added the Category: Dynamic/Personalized Content label Feb 3, 2018

ericlindley-g added the P3: When Possible label Apr 23, 2018

westonruter mentioned this issue May 3, 2018

Add AMP theme support Automattic/_s#1286

Open

dependabot-preview bot mentioned this issue May 3, 2018

Bump chai from 3.5.0 to 4.1.2 SKAHP/amphtml#31

Open

kevinkassimo mentioned this issue Jul 2, 2018

✨ Implement standard action toggleClass(...) #16530

Merged

4 tasks

dreamofabear mentioned this issue Jul 9, 2018

amp-bind: Easier toggling of classes #10622

Open

dreamofabear unassigned ericlindley-g Jul 9, 2018

dreamofabear changed the title ~~Toggling classes on click~~ amp-bind: Easier toggling of classes Jul 9, 2018

aghassemi closed this as completed in #16530 Jul 13, 2018

dreamofabear changed the title ~~amp-bind: Easier toggling of classes~~ Toggling classes on click Dec 6, 2018

henel677 mentioned this issue Apr 6, 2020

swg release 100 #27596

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Toggling classes on click #1032

Toggling classes on click #1032

cramforce commented Dec 1, 2015

rudygalfi commented Dec 1, 2015

AliberGzz commented Jan 12, 2016

sebastianbenz commented Feb 25, 2016

dvoytenko commented Feb 25, 2016

camelburrito commented Feb 25, 2016

sebastianbenz commented Feb 26, 2016

sebastianbenz commented Feb 26, 2016

dvoytenko commented Feb 26, 2016

rudygalfi commented Feb 27, 2016

rudygalfi commented Feb 18, 2017

ericlindley-g commented Feb 18, 2017

westonruter commented Feb 2, 2018

ericlindley-g commented Feb 2, 2018

dreamofabear commented Feb 3, 2018

westonruter commented May 3, 2018

westonruter commented May 3, 2018 •

edited

Loading

sebastianbenz commented May 3, 2018

westonruter commented May 3, 2018

westonruter commented May 3, 2018

sebastianbenz commented May 3, 2018

Toggling classes on click #1032

Toggling classes on click #1032

Comments

cramforce commented Dec 1, 2015

rudygalfi commented Dec 1, 2015

AliberGzz commented Jan 12, 2016

sebastianbenz commented Feb 25, 2016

dvoytenko commented Feb 25, 2016

camelburrito commented Feb 25, 2016

sebastianbenz commented Feb 26, 2016

sebastianbenz commented Feb 26, 2016

dvoytenko commented Feb 26, 2016

rudygalfi commented Feb 27, 2016

rudygalfi commented Feb 18, 2017

ericlindley-g commented Feb 18, 2017

westonruter commented Feb 2, 2018

ericlindley-g commented Feb 2, 2018

dreamofabear commented Feb 3, 2018

westonruter commented May 3, 2018

westonruter commented May 3, 2018 • edited Loading

sebastianbenz commented May 3, 2018

westonruter commented May 3, 2018

westonruter commented May 3, 2018

sebastianbenz commented May 3, 2018

westonruter commented May 3, 2018 •

edited

Loading