Skip to content

Related Posts: add docblock to existing filter#8359

Merged
oskosk merged 2 commits intoAutomattic:masterfrom
Umangvaghela:add-comment-in-apply-filter
Dec 20, 2017
Merged

Related Posts: add docblock to existing filter#8359
oskosk merged 2 commits intoAutomattic:masterfrom
Umangvaghela:add-comment-in-apply-filter

Conversation

@Umangvaghela
Copy link
Copy Markdown
Contributor

Changes proposed in this Pull Request:

  • Comment is not write on "jetpack_related_posts_customize_options filter" in class.ralated-posts-customize.php file so I add comment.

Testing instructions:

Proposed changelog entry for your changes:

@Umangvaghela Umangvaghela requested a review from a team as a code owner December 14, 2017 06:28
@jeherve jeherve changed the title add comment on apply filter Related Posts: add docblock to existing filter Dec 14, 2017
Copy link
Copy Markdown
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

Here are a few things you can do to make the filter easier to understand and use.

}

/**
* Filter that is used to customize related posts option.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you clarify things a bit here? The filter allows you to change the options used to display Related Posts in the Customizer. I think this explains things a bit more.

* Filter that is used to customize related posts option.
*
* @since 4.4.0
*
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you add @module related-posts here?

*
* @since 4.4.0
*
* @param mixed array post customize option.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should always be an array, so I would remove "mixed". I would also add the array variable, like so:
@param array $options Array of options used to display Related Posts in the Customizer.

@jeherve jeherve added the [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. label Dec 14, 2017
@Umangvaghela
Copy link
Copy Markdown
Contributor Author

@jeherve

Hello Sir,I update changes which suggested you. So please check it and provide your feedback if i miss something.

Thanks

@jeherve jeherve removed the [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. label Dec 14, 2017
Copy link
Copy Markdown
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

LGTM!

@jeherve jeherve added the [Status] Ready to Merge Go ahead, you can push that green button! label Dec 14, 2017
@oskosk oskosk added this to the 5.7 milestone Dec 20, 2017
Copy link
Copy Markdown
Contributor

@oskosk oskosk left a comment

Choose a reason for hiding this comment

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

LGTM!

Thanks for the changes @Umangvaghela !

@oskosk oskosk merged commit c4978c2 into Automattic:master Dec 20, 2017
@kraftbj kraftbj removed the [Status] Ready to Merge Go ahead, you can push that green button! label Oct 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants