Skip to content

Fix height of Contact Info map iframe in AMP#11443

Merged
jeherve merged 2 commits intoAutomattic:masterfrom
westonruter:add/contact-info-widget-amp-compat
Mar 22, 2019
Merged

Fix height of Contact Info map iframe in AMP#11443
jeherve merged 2 commits intoAutomattic:masterfrom
westonruter:add/contact-info-widget-amp-compat

Conversation

@westonruter
Copy link
Copy Markdown
Contributor

@westonruter westonruter commented Feb 28, 2019

See #9730.

Changes proposed in this Pull Request:

Since AMP has built-in responsiveness, the Contact Info widget's map iframe with 600⨉216 dimensions can the actual height to be much less. For example, in Twenty Seventeen the sidebar is 325px wide. This results in the iframe being 117px high, since 325:117 has the same aspect ratio as 600:216. To fix, this PR explicitly uses AMP's fixed-height layout for the amp-iframe so that the height will always be 216px with the width being responsive.

Testing instructions:

  1. Activate Twenty Seventeen.
  2. Install the AMP plugin and in the AMP settings switch to the paired mode. (You may also need to opt to hide the admin bar in AMP if there is too much CSS resulting.)
  3. Add the Contact Info widget and click the checkbox to show the map (which requires a Google Maps API key).
  4. Look at the frontend, and the non-AMP version should look like this:
screen shot 2019-02-28 at 11 17 01
  1. Switch to the AMP version (?amp) and in master it should look erroneously as this (with less vertical height):
screen shot 2019-02-28 at 11 17 12
  1. Switch to this branch and in the AMP version it should look like the following, which is the same as the non-AMP version:
screen shot 2019-02-28 at 11 25 43

The HTML markup for AMP should look like this (after ampproject/amp-wp#1913, without which the iframe inside the noscript is also converted to amp-iframe):

<amp-iframe layout="fixed-height" width="auto" sandbox="allow-scripts allow-same-origin" height="216" frameborder="0"
            src="https://www.google.com/maps/embed/v1/place?q=3999+Mission+Boulevard%2CSan+Diego+CA+92109&amp;key=..."
            class="contact-map">
    <span placeholder>Loading map…</span>
    <noscript>
        <iframe width="600" height="216" frameborder="0"
                src="https://www.google.com/maps/embed/v1/place?q=3999+Mission+Boulevard%2CSan+Diego+CA+92109&amp;key=..."
                class="contact-map"></iframe>
    </noscript>
</amp-iframe>

Proposed changelog entry for your changes:

  • Improve rendering of Contact Info widget map in AMP.

@jetpackbot
Copy link
Copy Markdown
Collaborator

jetpackbot commented Feb 28, 2019

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: April 2, 2019.
Scheduled code freeze: March 26, 2019

Generated by 🚫 dangerJS against b34de1b

@jeherve jeherve added Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Extra Sidebar Widgets AMP [Status] Needs Review This PR is ready for review. labels Feb 28, 2019
@jeherve jeherve added this to the 7.2 milestone Mar 4, 2019
kraftbj
kraftbj previously approved these changes Mar 11, 2019
Copy link
Copy Markdown
Contributor

@kraftbj kraftbj left a comment

Choose a reason for hiding this comment

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

Works for me as advertised and did not notice any adverse effects.

@kraftbj kraftbj added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Mar 13, 2019
@jeherve jeherve added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Mar 18, 2019
@kraftbj kraftbj added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Mar 22, 2019
@matticbot
Copy link
Copy Markdown
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello westonruter! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D25946-code before merging this PR. Thank you!

@jeherve jeherve merged commit e024812 into Automattic:master Mar 22, 2019
@ghost ghost removed the [Status] Ready to Merge Go ahead, you can push that green button! label Mar 22, 2019
kraftbj added a commit that referenced this pull request Mar 26, 2019
kraftbj added a commit that referenced this pull request Mar 27, 2019
* Initial Changelog for 7.2

* Testing list: add mention of IE11 testing

* Initial Changelog for 7.2

* Testing list: add mention of IE11 testing

* Add CL for #11224

* Add CL for #11426

* Add CL for #11442

* Add testing instructions for #11224

* Add CL for #11451

* Reclassify CL item

* Add testing instructions for #11451

* Add CL for #11486

* Add CL for #11418

* Add CL for #11524

* Add CL and testing instructions for #11449

* Add CL for #11460

* Add CL for #11520 and #11582

* Add CL for #11531

* Add CL #11644

* Add testing instructions for #11644

* Add testing instructions for #11644

* Add CL for #11618

* Uniform changelog lines

* CL #11679

* CL #11661

* CL #11654

* CL #11645

* CL #11643

* CL #11636

* CL #11635 and for other PHPCS commits

* CL #11627

* CL #11626

* CL #11598

* CL #11596

* Remove nested items for shortcopy. I don't believe the detailed list is helpful

* CL #11570

* CL #11569

* CL #11560

* CL #11558

* CL #11555

* CL #6704

* CL #11298

* CL #11324

* CL #11443

* CL #11484

* CL #11516

* CL #11529

* Expand Ads block enhancement CL item
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AMP Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Extra Sidebar Widgets Touches WP.com Files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants