Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add support in nav-menu-toggle-sanitizer for nav_container to be either the body or root element #2327

Merged
merged 5 commits into from
Jun 3, 2019
Merged

Add support in nav-menu-toggle-sanitizer for nav_container to be either the body or root element #2327

merged 5 commits into from
Jun 3, 2019

Conversation

felixarntz
Copy link
Collaborator

@felixarntz felixarntz commented May 15, 2019 •
edited by westonruter
Loading

It is possible to specify the body element as the target to modify the class for the navigation menu toggle to indicate whether the menu is currently toggled on or off. Several themes actually do this, because it allows the entire page (or more precisely everything in body) to be styled based on that class.

Currently this is broken because the current implementation will try to place the corresponding amp-state tag before the respective element, which in this case is between head and body which is obviously not valid.

This PR ensures that in case the body is specified the amp-state tag is inserted differently, as first child of body.

@felixarntz felixarntz added the Bug Something isn't working label May 15, 2019
@felixarntz felixarntz requested a review from westonruter May 15, 2019 21:35
@googlebot googlebot added the cla: yes Signed the Google CLA label May 15, 2019
@westonruter
Copy link
Member

westonruter commented May 15, 2019 •
edited
Loading

@felixarntz please rebase your commit on top of develop and likewise change the PR target to develop. This will fix the build error. Ref #2308.

@westonruter westonruter added this to the v1.1.2 milestone May 15, 2019
@swissspidy swissspidy changed the base branch from master to develop May 15, 2019 22:32
@swissspidy swissspidy force-pushed the fix/menu-toggle-sanitizer branch 2 times, most recently from 46e5383 to 24a88e9 Compare May 15, 2019 22:37
@westonruter
Copy link
Member

Please rebase to develop. I'll cherry-pick onto the 1.1 branch. I think you may have rebased against the 1.1 branch.

@swissspidy swissspidy force-pushed the fix/menu-toggle-sanitizer branch 3 times, most recently from 5454cc0 to 96ccc1e Compare May 15, 2019 22:39
@swissspidy
Copy link
Collaborator

Was passing by and thought I'll do it real quick. Messed it up at first, hence the force push, but now it should be good. Sorry for the noise 🙂

@westonruter westonruter force-pushed the fix/menu-toggle-sanitizer branch from 96ccc1e to 954c359 Compare May 15, 2019 22:43
@westonruter westonruter force-pushed the fix/menu-toggle-sanitizer branch from 954c359 to 340c53a Compare May 15, 2019 22:55
@felixarntz
Copy link
Collaborator Author

@swissspidy Awesome, thanks!

@westonruter

I like insertBefore rather than appendChild because it puts the amp-state closer to the element attributes that are being modified.

That was my thinking as well.

Just note that amp-auto-ads has docs that state:

We could consider accounting for that in the future, but since it's not a hard error, I think it's okay to move forward with this.

@westonruter
Copy link
Member

If you want to make use of toggleClass (perhaps in a subsequent PR), I have a PR for that: #2328.

@westonruter westonruter modified the milestones: v1.1.2, v1.2 May 20, 2019
@westonruter
Copy link
Member

If you want to make use of toggleClass (perhaps in a subsequent PR), I have a PR for that: #2328.

It looks like this would actually complicate things because of the need to update the aria-expanded attribute on the button, and using amp-bind is easiest here. Otherwise we'd have toggleClass managing the state in one place and amp-state storing the state in another.

@westonruter
Copy link
Member

westonruter commented May 21, 2019 •
edited
Loading

  • Would be good to add some tests for this as well.

@westonruter
Merge branch 'develop' of github.com:ampproject/amp-wp into fix/menu-…
35f9b0c 
…toggle-sanitizer

* 'develop' of github.com:ampproject/amp-wp: (1124 commits)
  Use rounded up constant for default height
  Only allow pan- animations for images
  Adjust style for font calculation.
  Bring back CSS file
  Remove now obsolete tests and assertions
  Merge amp-editor-blocks into amp-block-editor
  Fix issue with default height when using amp-fit-text
  Fix typo to unbreak height text control
  Remove unused closure import
  Do font size calculation for _all_ meta blocks
  Improve meta block height calculation
  Update min and max font sizes to match amp-fit-text
  Apply object-fit:cover to original size images, too.
  Hide original resize handles.
  Add tests for fixes to @import handling
  Fix bug with non-rejected @import statements being dropped
  Fix bug with replacing @import with contents
  Improve error messages for CSS @import failures
  Update dependency webpack to v4.32.2
  Update dependency eslint-plugin-jest to v22.6.4
  ...
@westonruter
Copy link
Member

Build for testing: amp.zip (1.2-beta1-20190523T165154Z-35f9b0c6)

@westonruter westonruter mentioned this pull request May 23, 2019
Closed
@felixarntz felixarntz self-assigned this May 28, 2019
@felixarntz
Copy link
Collaborator Author

I can get to adding tests either this Friday or early next week.

@felixarntz
Copy link
Collaborator Author

@westonruter Tests are now present, this is good for merge from my end.

@swissspidy swissspidy requested a review from westonruter June 3, 2019 13:54
@westonruter westonruter changed the title Prevent amp-state for nav menu toggle being inserted outside of body Add support in nav-menu-toggle-sanitizer for nav_container to be either the body or root element Jun 3, 2019
@westonruter westonruter merged commit 231098e into develop Jun 3, 2019
@swissspidy swissspidy deleted the fix/menu-toggle-sanitizer branch June 4, 2019 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@swissspidy swissspidy swissspidy approved these changes

@westonruter westonruter westonruter approved these changes

Assignees

@felixarntz felixarntz

Labels
Bug Something isn't working cla: yes Signed the Google CLA
Projects
None yet
Milestone
v1.2
Development

Successfully merging this pull request may close these issues.
4 participants
@felixarntz @westonruter @swissspidy @googlebot