Skip to content

Forms: deprecate outlined and animated form styles. #43366

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

Open
wants to merge 2 commits into
base: update/migrate-form-fields-to-inner-blocks
Choose a base branch
from

Conversation

ramonjd
Copy link
Member

@ramonjd ramonjd commented May 5, 2025

Related to #43327

Proposed changes:

Deprecates Outlined and Animated styles (in fact, all styles) and removes them when a legacy form is loaded in the editor.

When a form is opened using this branch, the is-style-outline is detectted upon load.

Only if you save the editor does the change take place on the frontend.

Screenshot 2025-05-06 at 2 46 19 pm

Other information:

  • Have you written new tests for your changes, if applicable?
  • Have you checked the E2E test CI results, and verified that your changes do not break them?
  • Have you tested your changes on WordPress.com, if applicable (if so, you'll see a generated comment below with a script to run)?

Jetpack product discussion

Does this pull request change what data or activity we track or use?

Testing instructions:

  • Go to '..'

@ramonjd ramonjd changed the base branch from trunk to update/migrate-form-fields-to-inner-blocks May 5, 2025 06:35
Copy link
Contributor

github-actions bot commented May 5, 2025

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

  • ✅ Include a description of your PR changes.
  • 🔴 Add a "[Status]" label (In Progress, Needs Review, ...).
  • 🔴 Add a "[Type]" label (Bug, Enhancement, Janitorial, Task).
  • ✅ Add testing instructions.
  • ✅ Specify whether this PR includes any changes to data or privacy.
  • 🔴 Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖


🔴 Action required: Please add missing changelog entries for the following projects: projects/packages/forms

Use the Jetpack CLI tool to generate changelog entries by running the following command: jetpack changelog add.
Guidelines: /docs/writing-a-good-changelog-entry.md


Follow this PR Review Process:

  1. Ensure all required checks appearing at the bottom of this PR are passing.
  2. Make sure to test your changes on all platforms that it applies to. You're responsible for the quality of the code you ship.
  3. You can use GitHub's Reviewers functionality to request a review.
  4. When it's reviewed and merged, you will be pinged in Slack to deploy the changes to WordPress.com simple once the build is done.

If you have questions about anything, reach out in #jetpack-developers for guidance!

@github-actions github-actions bot 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 May 5, 2025
@@ -103,6 +104,8 @@ function JetpackContactFormEdit( { name, attributes, setAttributes, clientId, cl
const innerRef = useRef();
const blockProps = useBlockProps( { ref: wrapperRef } );
const formClassnames = clsx( className, 'jetpack-contact-form' );
const hasUpdatedFormStyle = useHasDeprecatedFormStyle( clientId );
console.log( 'hasUpdatedFormStyle', hasUpdatedFormStyle );
Copy link
Member Author

Choose a reason for hiding this comment

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

Remove this. 😄

// { name: 'animated', label: __( 'Animated', 'jetpack-forms' ) },
// @deprecated
// { name: 'outlined', label: __( 'Outlined', 'jetpack-forms' ) },
// @TODO check if this is needed. It was added 2 years ago and never activated. Time to delete associated styles?
Copy link
Member Author

Choose a reason for hiding this comment

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

Let's get rid of the "below" style and associated code in another PR.

Comment on lines 101 to 104
// @deprecated
// { name: 'animated', label: __( 'Animated', 'jetpack-forms' ) },
// @deprecated
// { name: 'outlined', label: __( 'Outlined', 'jetpack-forms' ) },
Copy link
Member Author

Choose a reason for hiding this comment

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

To keep things need we could extricate all styles and code related to the styles and place them in a deprecated folder.

@talldan
Copy link
Contributor

talldan commented May 5, 2025

What you're seeing in this screencast is a form built in trunk using the Outlined style. When opened using this branch, the is-style-outline is remove automatically upon load.

I personally think we can retain the class, so it becomes only a custom class name that the user can still update in the 'advanced' block tools (I think the classnames are already visible there, so I think it already works).

Otherwise the user might be making a small unrelated edit to their form (e.g. changing another block or the title of their post), and then find their form styles are changed when saving.

I think the only complex thing would be if we introduce other style variations in the future, and then the user ends up mixing is-style-outlined with another block style.

@ramonjd
Copy link
Member Author

ramonjd commented May 5, 2025

I personally think we can retain the class, so it becomes only a custom class name that the user can still update in the 'advanced' block tools (I think the classnames are already visible there, so I think it already works).

This is what I originally did. The form looked completely borked.

I think it's because when removing the styles from the registration data in index.js, the block state no longer knows if the form has the style or not. And because the style determines the HTML, e.g., it conditionally loads all the notched HTML, everything explodes.

I haven't spent much time yet looking around, but I suppose we could detect the styles on the className attributes and load the notched HTML.

I'll try that tomorrow.

@talldan
Copy link
Contributor

talldan commented May 5, 2025

This is what I originally did. The form looked completely borked.

In the editor or frontend? I think in the editor we could consider completely removing the is-style-outlined / is-style-animated css along with that code that adds the extra markup. The editor wouldn't look like the front-end, but that's the benefit to us of deprecating it IMO. Or alternatively we keep some similar styles that look close, but not completely the same.

On the frontend I think it's this code here -

private function get_form_style() {
$class_name = $this->form->get_attribute( 'className' );
preg_match( '/is-style-([^\s]+)/i', $class_name, $matches );
return count( $matches ) >= 2 ? $matches[1] : null;
}
/**
* Checks if the field has an inset label, i.e., a label displayed inside the field instead of above.
*
* @return boolean
*/
private function has_inset_label() {
$form_style = $this->get_form_style();
return in_array( $form_style, array( 'outlined', 'animated' ), true );
}
}

@ramonjd
Copy link
Member Author

ramonjd commented May 5, 2025

In the editor or frontend?

The editor.

The editor wouldn't look like the front-end, but that's the benefit to us of deprecating it IMO.

So removing the notched HTML and associated style CSS in the editor should work to achieve this I'm guessing. I'll try that next.

We'll leave the frontend as is of course for users that don't update their forms.

@ramonjd
Copy link
Member Author

ramonjd commented May 5, 2025

I think in the editor we could consider completely removing the is-style-outlined / is-style-animated css along with that code that adds the extra markup. The editor wouldn't look like the front-end, but that's the benefit to us of deprecating it IMO.

I tried retaining the style classname (is-style-outline and is-style-animated) on the form block where previously applied, but it's a bit confusing as there's no obvious way for the user to remove it to make the editor and frontend consistent.

The editor and frontend will continue to look differently until the user removes the classname from the advance panel in the form's block settings.

Maybe it's just me, and it's not a problem. 😄

Anyway, here I've removed the styles from the registration object, and removed all editor CSS and HTML related to the animated/outline styles.

Kapture.2025-05-05.at.19.38.26.mp4

I generally think it's okay, but it might require some nudge in the editor? What do folks think?

@ramonjd
Copy link
Member Author

ramonjd commented May 5, 2025

The editor and frontend will continue to look differently until the user removes the classname from the advance panel in the form's block settings.

Maybe it's just me, and it's not a problem. 😄

Withdraw that. It is a problem.

I think what we're missing with this solution is the issue we were trying to fix in the first place with #43366: if you change the border styles of input blocks, they won't be reflected on the frontend.

So I think we're compounding confusion here: we're deprecating the style, keeping the class name around in the editor, but retaining all the bugs.

@talldan
Copy link
Contributor

talldan commented May 6, 2025

Withdraw that. It is a problem.

I think what we're missing with this solution is the issue we were trying to fix in the first place with #43366: if you change the border styles of input blocks, they won't be reflected on the frontend.

So I think we're compounding confusion here: we're deprecating the style, keeping the class name around in the editor, but retaining all the bugs.

I guess we could keep the frontend solution from the other PR to keep borders working, since that wasn't as complex as the editor solution. 🤔

Another addition could be a small notice in the style variations UI explaining that a deprecated style is being used (hopefully there's a inspector controls group for that). The notice could also provide the option to remove the classname through a button.

Maybe you're right though, the experience could be too broken to try deprecating these styles.

I do think exploring this PR gives a better idea of the trade offs. The situation with these styles is pretty messy, thanks for continuing to look into it!

@ramonjd
Copy link
Member Author

ramonjd commented May 6, 2025

Another addition could be a small notice in the style variations UI explaining that a deprecated style is being used (hopefully there's a inspector controls group for that). The notice could also provide the option to remove the classname through a button.

That's a good compromise, at least for this PR. I'll update accordingly so folks can compare.

Mentioned on the other PR, but a refactor is possible with a few trade offs (label needs a background color to "break" the input border):

https://codepen.io/ramonjd/pen/ByyxExd

Thanks for the continued feedback!

@ramonjd ramonjd changed the title Forms: deprecate outlinedand animated form styles. Forms: deprecate outlined and animated form styles. May 6, 2025
Copy link
Contributor

github-actions bot commented May 6, 2025

Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.

  • To test on WoA, go to the Plugins menu on a WoA dev site. Click on the "Upload" button and follow the upgrade flow to be able to upload, install, and activate the Jetpack Beta plugin. Once the plugin is active, go to Jetpack > Jetpack Beta, select your plugin (Jetpack or WordPress.com Site Helper), and enable the try/deprecate-outline-animated-form-styles branch.
  • To test on Simple, run the following command on your sandbox:
bin/jetpack-downloader test jetpack try/deprecate-outline-animated-form-styles
bin/jetpack-downloader test jetpack-mu-wpcom-plugin try/deprecate-outline-animated-form-styles

Interested in more tips and information?

  • In your local development environment, use the jetpack rsync command to sync your changes to a WoA dev blog.
  • Read more about our development workflow here: PCYsg-eg0-p2
  • Figure out when your changes will be shipped to customers here: PCYsg-eg5-p2

Copy link

jp-launch-control bot commented May 6, 2025

Code Coverage Summary

No summary data is available for parent commit 8c63530, so cannot calculate coverage changes. 😴

If that commit is a feature branch rather than a trunk commit, this is expected. Otherwise, this should be updated once coverage for 8c63530 is available.

Full summary · PHP report · JS report

@andrewserong andrewserong force-pushed the update/migrate-form-fields-to-inner-blocks branch 2 times, most recently from e805792 to 8c63530 Compare May 7, 2025 01:09
@ramonjd ramonjd force-pushed the try/deprecate-outline-animated-form-styles branch from bc8dbeb to 28c431d Compare May 7, 2025 06:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Contact Form Form block (also see Contact Form label) [Feature] Contact Form [Package] Forms [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants