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

[RNMobile] Update code comment for clarity #16547

Merged
merged 5 commits into from
May 16, 2022

Conversation

SiobhyB
Copy link
Contributor

@SiobhyB SiobhyB commented May 13, 2022

Addresses feedback received at #16507 (comment)

In #16507, a conditional that communicates whether xPosts are enabled on a site was updated to check whether a site was hosted on WordPress.com. The reason for this change is that the conditional's mIsXPostsCapable boolean returns true for .org sites by default. As the app only support xPosts for WordPress.com sites using a P2-based theme, an unresponsive + icon was being displayed in the editor's toolbar for .org sites.

The updated conditional, including comment, was as follows:

// If this.mIsXPostsCapable has not been set, default to allowing xPosts
boolean enableXPosts = mSite.isUsingWpComRestApi() && (mIsXPostsCapable == null || mIsXPostsCapable);

This feedback suggested that the comment could be updated for clarity, given the change. As such, this PR proposes the following change:

// this.mIsXPostsCapable may return true for non-WP.com sites, but the app only supports xPosts for P2-based
// WP.com sites so, gate with `isUsingWpComRestApi()`
// If this.mIsXPostsCapable has not been set, default to allowing xPosts.

Regression Notes

  1. Potential unintended areas of impact

As this is a change to a code comment, no user-facing regressions are predicted. The only negative, unintentional impact would be developer confusion, if the comment isn't clear.

  1. What I did to test those areas of impact (or what existing automated tests I relied on)

N/A

  1. What automated tests I added (or what prevented me from doing so)

N/A

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented May 13, 2022

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@SiobhyB SiobhyB added Gutenberg Editing and display of Gutenberg blocks. gutenberg-mobile labels May 13, 2022
@SiobhyB SiobhyB self-assigned this May 13, 2022
@SiobhyB SiobhyB changed the title [RNMobile] Update comment for clarity [RNMobile] Update code comment for clarity May 13, 2022
@SiobhyB SiobhyB added this to the 19.9 milestone May 13, 2022
@SiobhyB SiobhyB marked this pull request as ready for review May 13, 2022 15:50
@SiobhyB SiobhyB requested a review from hypest May 13, 2022 15:50
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented May 13, 2022

You can test the changes on this Pull Request by downloading the APKs:

Siobhan and others added 4 commits May 13, 2022 17:16
"If the site is using the WP.com API..." was too verbose, especially considering that was evident from reading the conditional itself. It's therefore been removed with this comment. Instead, the previous sentence will be iterated on in a following commit to clarify why the 'isUsingWpComRestApi' is used.
"If the site is using the WP.com API..." was too verbose, especially considering that was evident from reading the conditional itself. It's therefore been removed with this comment. Instead, the previous sentence will be iterated on in a following commit to clarify why the 'isUsingWpComRestApi' is used.
Co-authored-by: Stefanos Togoulidis <stefanostogoulidis@gmail.com>
Copy link
Contributor

@hypest hypest left a comment

Choose a reason for hiding this comment

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

LGTM!

@hypest hypest merged commit 5d6fcc4 into trunk May 16, 2022
@hypest hypest deleted the gutenberg/chore/update-xpost-comment-for-clarification branch May 16, 2022 05:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Gutenberg Editing and display of Gutenberg blocks.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants