wp-calypso icon indicating copy to clipboard operation
wp-calypso copied to clipboard

A4A-Partner Directory: Highlight invalid form fields

Open cleacos opened this issue 1 year ago • 7 comments

Resolves https://github.com/Automattic/automattic-for-agencies-dev/issues/716

Proposed Changes

image

This PR highlights the invalid field values. In this case, we check/validate the URLs and the email.

The validator highlight text will be displayed if you type at least three (3) characters.

Note

In a follow-up PR, the Agency Details form could be saved at any moment without completing the whole form, even with no valid fields.

  • In another PR, on each field update, the details form will be saved.

Why are these changes being made?

Testing Instructions

  • Check the code
  • Try to introduce an invalid URL (Any form field for URL, in the Details and Expertise form)
  • Try to introduce an invalid Email (Company email, in the Details form)

Pre-merge Checklist

  • [ ] Has the general commit checklist been followed? (PCYsg-hS-p2)
  • [ ] Have you written new tests for your changes?
  • [ ] Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
  • [ ] Have you checked for TypeScript, React or other console errors?
  • [ ] Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
  • [ ] Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
  • [ ] For changes affecting Jetpack: Have we added the "[Status] Needs Privacy Updates" label if this pull request changes what data or activity we track or use (p4TIVU-aUh-p2)?

cleacos avatar Jun 26 '24 11:06 cleacos

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

Sections (~1111 bytes added 📈 [gzipped])

name                                parsed_size           gzip_size
a8c-for-agencies-partner-directory      +3724 B  (+0.8%)    +1059 B  (+0.7%)
a8c-for-agencies-sites                   +230 B  (+0.0%)      +52 B  (+0.0%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

matticbot avatar Jun 26 '24 11:06 matticbot

Looks good so far, @cleacos.

Have a few comments on the behavior and on styling though, so leaving it up to you whether to address here or on a separate PR.

Behavior

The fields are marked as soon as it detects something is off, which is not a great experience. In other words, it points out the error even before a mistake happens. See this example where I'm trying to add a URL:

https://github.com/Automattic/wp-calypso/assets/390760/8dc86b0c-6609-42f2-9a50-58bb5ca75a88

Now check this example where it delays the error for when the input is de-selected (or onblur). Please note that this was the first example I found, but the particular implementation on this Codepen does not matter for our case.

https://github.com/Automattic/wp-calypso/assets/390760/9df950ea-55ed-4a70-acbb-a3d9bcbdb8d4

From: https://codepen.io/bramus/pen/ExNYBOK

Styling

In terms of styling, I think we can get rid of the parenthesis and also outline the entire field with the red color. That would look something like this:

image

Additionally, and if possible at all, we should try to position the error message below the field so that it's more obvious something needs attention. This is what we're doing in Calypso:

image

Thanks, Clemente!

keoshi avatar Jun 26 '24 15:06 keoshi

I completely agree with using the Calypso pattern to ensure consistency when displaying errors. We have already implemented this pattern on the 'Add Payment Method' page.

Screenshot 2024-06-26 at 11 44 48 PM

jkguidaven avatar Jun 26 '24 15:06 jkguidaven

@keoshi @cleacos @jkguidaven Can you give it a go. I've updated to follow signup pattern.

Screenshot 2024-06-26 at 12 44 32 PM

andrii-lysenko avatar Jun 26 '24 20:06 andrii-lysenko

Thank you @andrii-lysenko for working on it. I'll take a look and take over on it.

cleacos avatar Jun 27 '24 07:06 cleacos

This PR modifies the release build for the following Calypso Apps:

For info about this notification, see here: PCYsg-OT6-p2

  • blaze-dashboard
  • command-palette-wp-admin
  • help-center
  • notifications
  • odyssey-stats
  • wpcom-block-editor

To test WordPress.com changes, run install-plugin.sh $pluginSlug add/a4a/partner-directory/highlight-invalid-form-fields on your sandbox.

matticbot avatar Jun 28 '24 17:06 matticbot

Would still try to move validation to onblur on each field instead of having to click the Submit button

I was working on it, I prefer that solution too, onBlur... Since this PR is not super prioritize, I'll push this for tomorrow.

cleacos avatar Jul 01 '24 14:07 cleacos

After talking with @andrii-lysenko, we will address the right way to this, using the onBlur property and wrapping all this logic inside of the new A4AFormFieldset component.

With this version, the user cannot save the profile details until all the data is valid. The ideal behavior is that the user can save the profile information, but until we don't have the validation per each field instead of the full form, we don't have a way to highlight the wrong field. It's a short form, it's the MVP version, and highlighting what is the wrong field seems more important. We will address the right way in a follow-up PR. cc: @jeffgolenski

cleacos avatar Jul 03 '24 17:07 cleacos

This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/16058124

Some locales (Hebrew) have been temporarily machine-translated due to translator availability. All other translations are usually ready within a few days. Untranslated and machine-translated strings will be sent for translation next Monday and are expected to be completed by the following Friday.

Thank you @cleacos for including a screenshot in the description! This is really helpful for our translators.

a8ci18n avatar Jul 04 '24 00:07 a8ci18n

Translation for this Pull Request has now been finished.

a8ci18n avatar Jul 11 '24 10:07 a8ci18n