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

Revue Block: Minor Fixes #14792

Merged
merged 3 commits into from
Feb 26, 2020
Merged

Revue Block: Minor Fixes #14792

merged 3 commits into from
Feb 26, 2020

Conversation

Copons
Copy link
Contributor

@Copons Copons commented Feb 25, 2020

Changes proposed in this Pull Request:

  • Make the input fields block elements and responsive.
  • Trim the Revue username on save to prevent issues caused by unwanted whitespaces.
  • Move the Edit Username flow to the block sidebar.
  • Dynamically create the CSS classes in the SSR.

These minor fixes were partly requested in the post-merge comments on the original Revue Block PR (starting from this comment).

Screenshots

In some themes, labels and input fields are inline elements. In others, like Twenty Twenty, they are block elements.
25d3f05 makes them block elements regardless.
The following comparison screenshots contains the Revue block followed by the Contact Form block, which is the look we are trying to obtain.

Before After
Screenshot 2020-02-25 at 11 30 29 Screenshot 2020-02-25 at 11 32 10

1dc853c removes the "Edit Username" button from the block toolbar that reverted the block to the placeholder view, and replaces it with a simple text field in the block sidebar.

Screenshot 2020-02-25 at 11 39 30

Testing instructions:

  • Insert a Revue block and save the post.
  • Switch to a theme with inline labels and inputs (Mayland and Maywood, for example), and make sure the inputs in the rendered block are (CSS) block element.
  • In the editor, make sure there is no "Edit username" button in the block toolbar, and instead there is an input field in the block sidebar.
  • Make sure that input field works as expected.
  • Try the subscription form, and make sure it still works even after the CSS class changes. In particular, the form should be replaced by a confirmation message on submit.

Proposed changelog entry for your changes:

  • N/A (block unreleased)

@Copons Copons added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Status] Needs Review To request a review from Crew. Label will be renamed soon. [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack [Block] Revue labels Feb 25, 2020
@Copons Copons requested a review from a team February 25, 2020 12:30
@Copons Copons requested a review from a team as a code owner February 25, 2020 12:30
@Copons Copons self-assigned this Feb 25, 2020
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello Copons! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D39326-code before merging this PR. Thank you!

@jetpackbot
Copy link

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: March 3, 2020.
Scheduled code freeze: February 25, 2020

Generated by 🚫 dangerJS against 4edfe35

@jeherve jeherve added this to the 8.3 milestone Feb 25, 2020
jeherve
jeherve requested changes Feb 26, 2020
View reviewed changes
Copy link
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.

This works for me. I only have one minor remark, unlinked to this PR, but since we're bundling a few changes in this PR already, I think it would make sense to fix that too.

extensions/blocks/revue/revue.php
@@ -35,16 +35,18 @@ function jetpack_render_revue_block( $attributes ) {
$last_name_placeholder = jetpack_get_revue_attribute( 'lastNamePlaceholder', $attributes );
$last_name_show = jetpack_get_revue_attribute( 'lastNameShow', $attributes );
$url = sprintf( 'https://www.getrevue.co/profile/%s/add_subscriber', $attributes['revueUsername'] );
Copy link
Member

Choose a reason for hiding this comment

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

I know that's not part of this PR, but I think we need to escape this username. I had missed that before, sorry!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We escape the URL a few lines later though: action="<?php echo esc_url( $url ); ?>"
Do I still need to escape the username here too? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

True. I just realized others are having the same conversation elsewhere. :)
#14590 (comment)

Let's keep things as they are in here then. Ideally, maybe we should validate the username to avoid the use of characters like / for example.

@jeherve jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Feb 26, 2020
@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Feb 26, 2020
@jeherve jeherve merged commit 64db1ea into master Feb 26, 2020
@jeherve jeherve deleted the update/revue-block-fixes branch February 26, 2020 17:21
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Feb 26, 2020
jeherve pushed a commit that referenced this pull request Feb 26, 2020
@Copons @jeherve
[not verified] Revue Block: Minor Fixes (#14792)
0e95ce6 
* Proper block and responsive inputs

* Move username field in the sidebar

* Create CSS classes dynamically
@jeherve
Copy link
Member

jeherve commented Feb 26, 2020

Cherry-picked to branch-8.3 in 0e95ce6

@Copons
Copy link
Contributor Author

Copons commented Feb 26, 2020

r203456-wpcom

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scruffian scruffian scruffian approved these changes

@jeherve jeherve jeherve approved these changes

Assignees

@Copons Copons

Labels
[Block] Revue [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack [Pri] High Touches WP.com Files [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Milestone
8.3
Development

Successfully merging this pull request may close these issues.
5 participants
@Copons @matticbot @jetpackbot @jeherve @scruffian