Skip to content

Fix expanded menu when screen width is <1000px for Twenty Twenty theme#3790

Merged
westonruter merged 2 commits intodevelopfrom
fix/lightbox_small_screen_width
Nov 19, 2019
Merged

Fix expanded menu when screen width is <1000px for Twenty Twenty theme#3790
westonruter merged 2 commits intodevelopfrom
fix/lightbox_small_screen_width

Conversation

@pierlon
Copy link
Copy Markdown
Contributor

@pierlon pierlon commented Nov 18, 2019

Summary

With the Twenty Twenty theme, when the expanded menu is active on a screen width <1000px, it is incorrectly shown:

Before

image

The root cause of this is due to the container that the menu is placed in by amp-lightbox upon initialization is not made to fill the content because the amp-lightbox has the scrollable attribute.

This PR fixes this issue by unsetting the display CSS property of the amp-lightbox, reverting it from flex to block.

After

image

Checklist

  • My pull request is addressing an open issue (please create one otherwise).
  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@googlebot googlebot added the cla: yes Signed the Google CLA label Nov 18, 2019
@westonruter westonruter added this to the v1.4.2 milestone Nov 18, 2019
@pierlon pierlon self-assigned this Nov 18, 2019
Copy link
Copy Markdown
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fixing the issue for me.


@media (max-width: 999px) {
.show-modal {
display: unset !important;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to avoid the use of !important if at all possible.

Can you try with the following, more specific selector: amp-lightbox.cover-modal.show-modal ? I think that should fix the problem as well without using !important.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure that works and would prevent future CSS rule conflicts.

@westonruter westonruter dismissed schlessera’s stale review November 19, 2019 16:57

Use of !important has been removed

@westonruter westonruter merged commit f157602 into develop Nov 19, 2019
@westonruter westonruter deleted the fix/lightbox_small_screen_width branch November 19, 2019 16:57
westonruter pushed a commit that referenced this pull request Nov 19, 2019
#3790)

* Fix amp-lightbox when screen width is <1000px for Twenty Twenty theme

* Use a more specific selector rather than using `!important` to apply the rule
westonruter added a commit that referenced this pull request Nov 20, 2019
…ader-mode-templates

* 'develop' of github.com:ampproject/amp-wp:
  Ensure Stories preview meets minimum viewport requirements. (#3797)
  Fix expanded menu when screen width is <1000px for Twenty Twenty theme (#3790)
  Ignore test utils from code coverage
@csossi
Copy link
Copy Markdown

csossi commented Dec 7, 2019

Verified in QA

@csossi csossi added the QA passed Has passed QA and is done label Feb 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes Signed the Google CLA QA passed Has passed QA and is done

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants