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

AN-55 Fix for widescreen text width #263

Merged
merged 6 commits into from Dec 30, 2016
Merged

Conversation

kevinfodness
Copy link
Member

  • Sets the body text column span on non-center aligned layouts to be the same as the number of columns so that the text goes edge-to-edge.
  • Fixes the logic for positioning images in centered and non-centered layouts to prevent bleed-over from the margins on non-centered layouts.
  • Refactors the following classes for adherence to WordPress standards and PHP best practices:
    • Apple_Exporter\Builders\Builder
    • Apple_Exporter\Settings

Fixes #233.

Copy link
Member

@jakewrfoster jakewrfoster left a comment

Choose a reason for hiding this comment

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

nothing blocking, just a consideration and a missing phpdoc
👏 🐴

protected function content_nodes() {
return $this->content->nodes();
protected function content_title() {
return $this->content->title() ?: __( 'Untitled Article', 'apple-news' );
Copy link
Member

Choose a reason for hiding this comment

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

Not sure this is a valid concern, but seeing that WP is backward compat to php 5.2.x, this might be an issue (though highly unlikely). Support for this shorthand syntax for ternary operations was introduced in 5.3.

Not sure this is a risk, just thought I'd flag it since this is a plugin.

*/
protected function get_setting( $name ) {
return $this->settings->get( $name );
protected function set_content_property( $name, $value ) {
Copy link
Member

Choose a reason for hiding this comment

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

missing @param for these guys

@kevinfodness kevinfodness merged commit a63302a into master Dec 30, 2016
@kevinfodness kevinfodness deleted the AN-55-widescreen-text-width branch December 30, 2016 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants