Skip to content

Subscriptions: display the checkboxes for logged in users too.#11149

Merged
jeherve merged 1 commit intomasterfrom
fix/subscriptions-logged-in-checkboxes
Jan 25, 2019
Merged

Subscriptions: display the checkboxes for logged in users too.#11149
jeherve merged 1 commit intomasterfrom
fix/subscriptions-logged-in-checkboxes

Conversation

@jeherve
Copy link
Copy Markdown
Member

@jeherve jeherve commented Jan 16, 2019

Changes proposed in this Pull Request:

In #9983, we changed the way the subscription fields were hooked into the comment form.
The comment_form_after_fields action we started using is not triggered for logged in users.

comment_form_submit_button seems to be the only better filter right now.
I also added some CSS so the fields are never too close of the submit button.

Testing instructions:

  • Disable the Comment Form module.
  • In Settings > Discussion, make sure the subscription checkboxes are set.
  • Visit a post when logged in, and when logged out.
  • The checkboxes should appear above the comment submit button in both cases.

Proposed changelog entry for your changes:

  • Subscriptions: display the checkboxes for logged in users too.

In #9983, we changed the way the subscription fields were hooked into the comment form.
The comment_form_after_fields action we started using is not triggered for logged in users.

comment_form_submit_button seems to be the only better filter right now.
I also added some CSS so the fields are never too close of the submit button.
@jeherve jeherve added Bug When a feature is broken and / or not performing as intended [Feature] Subscriptions All subscription-related things such as paid and unpaid, user management, and newsletter settings. [Status] Needs Review This PR is ready for review. labels Jan 16, 2019
@jeherve jeherve added this to the 7.0 milestone Jan 16, 2019
@jeherve jeherve self-assigned this Jan 16, 2019
@jeherve jeherve requested a review from a team January 16, 2019 11:31
@jetpackbot
Copy link
Copy Markdown
Collaborator

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: February 5, 2019.
Scheduled code freeze: January 29, 2019

Generated by 🚫 dangerJS against 9c1329b

Copy link
Copy Markdown
Contributor

@brbrr brbrr left a comment

Choose a reason for hiding this comment

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

Checkboxes now visible for logged in users :shipit:

@brbrr brbrr added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Jan 16, 2019
@jeherve jeherve merged commit 432edb7 into master Jan 25, 2019
@ghost ghost removed the [Status] Ready to Merge Go ahead, you can push that green button! label Jan 25, 2019
@jeherve jeherve deleted the fix/subscriptions-logged-in-checkboxes branch January 25, 2019 16:45
jeherve added a commit that referenced this pull request Jan 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug When a feature is broken and / or not performing as intended [Feature] Subscriptions All subscription-related things such as paid and unpaid, user management, and newsletter settings.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants