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

VIP Review Changes #953

Merged
merged 6 commits into from Feb 28, 2018
Merged

VIP Review Changes #953

merged 6 commits into from Feb 28, 2018

Conversation

philipjohn
Copy link
Contributor

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!

@westonruter
Copy link
Member

westonruter commented Feb 11, 2018 •
edited

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 February 11, 2018 18:53
@westonruter westonruter changed the base branch from 0.6 to develop February 11, 2018 18:54
@westonruter westonruter changed the base branch from develop to 0.6 February 11, 2018 18:57
@westonruter
Copy link
Member

Rebased against 0.6 branch. Former HEAD was 3c74fe2.

westonruter and others added 3 commits February 11, 2018 11:34
@westonruter
Limit flushing rewrite rules to only when supported_post_types change
e3ac098 
Also fix phpcs issues
@zigasancin @westonruter
Fix AMP preview icon in Firefox
5555918 
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
Bump 0.6.2 and update readme
664a2a1
@westonruter
Copy link
Member

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

@philipjohn
Copy link
Contributor Author

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
Copy link
Member

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

ThierryA
ThierryA approved these changes Feb 20, 2018
View reviewed changes
Copy link
Collaborator

@ThierryA ThierryA left a comment

Choose a reason for hiding this comment

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

Great stuff, all good to go!

@westonruter
Copy link
Member

OK, release tomorrow morning?

@ThierryA ThierryA merged commit e18d560 into 0.6 Feb 28, 2018
@westonruter westonruter deleted the fix/vip-review branch July 5, 2018 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@westonruter westonruter westonruter approved these changes

@ThierryA ThierryA ThierryA approved these changes

@amedina amedina Awaiting requested review from amedina

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@philipjohn @westonruter @ThierryA @zigasancin