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.

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

VIP Review Changes #953

Merged
merged 6 commits into from Feb 28, 2018

Conversation

Projects
None yet
4 participants
@philipjohn
Copy link
Contributor

philipjohn commented Feb 10, 2018

Having completed a review, the following feedback came up:

  • includes/options/views/class-amp-analytics-options-submenu-page.php:24 - This line should just move into the preceeding else statement.
  • includes/options/views/class-amp-analytics-options-submenu-page.php:60 - The output should be escaped.
  • includes/templates/class-amp-post-template.php:97 - We should check if there is a post.

This PR addresses all that feedback!

@philipjohn philipjohn requested review from westonruter, amedina and ThierryA Feb 10, 2018

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Feb 11, 2018

To be included in a 0.6.x release, these commits should be re-based off of the 0.6 branch and the PR opened into the 0.6 branch.

@westonruter westonruter changed the base branch from develop to 0.6 Feb 11, 2018

@westonruter westonruter changed the base branch from 0.6 to develop Feb 11, 2018

@westonruter westonruter force-pushed the fix/vip-review branch from 3c74fe2 to fc862f5 Feb 11, 2018

@westonruter westonruter changed the base branch from develop to 0.6 Feb 11, 2018

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Feb 11, 2018

Rebased against 0.6 branch. Former HEAD was 3c74fe2.

westonruter and others added some commits Feb 11, 2018

Fix AMP preview icon in Firefox
The AMP preview icon isn't displaying correctly in Firefox (probably because of a different interpretation of the text-indent css rule). My proposed fix works in latest releases of Firefox, Chrome, Safari, Edge and IE.
@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Feb 11, 2018

Let's use this PR for the 0.6.2 release. I've cherry-picked #920 into this branch as well.

@philipjohn

This comment has been minimized.

Copy link
Contributor Author

philipjohn commented Feb 11, 2018

Thanks for doing the rebase 👍 I'll keep my eye on any changes here so that we can ship 0.6.2 out to VIP as soon as it's ready.

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Feb 20, 2018

@ThierryA anything you'd like to change prior to merging this and doing the 0.6.2 release?

@ThierryA
Copy link
Collaborator

ThierryA left a comment

Great stuff, all good to go!

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Feb 27, 2018

OK, release tomorrow morning?

@ThierryA ThierryA merged commit e18d560 into 0.6 Feb 28, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the fix/vip-review branch Jul 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.