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

Site Migration: Preserve urlValue in text input when an invalid URL is submitted #89546

Closed
wants to merge 9 commits into from

Conversation

sixhours
Copy link
Contributor

@sixhours sixhours commented Apr 15, 2024

Related to paYKcK-4n3-p2#comment-3248

Proposed Changes

  • Hide the error message when the text input is blank
  • Preserve the input when a URL is invalidated so the user can see what they entered.

Testing Instructions

  • Switch to this PR
  • Go through the onboarding flow from /start
  • Choose import as your site intent
  • TBD

Pre-merge Checklist

  • Has the general commit checklist been followed? (PCYsg-hS-p2)
  • https://wpcalypso.wordpress.com/devdocs/docs/testing/index.md 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)?

@sixhours sixhours added the [Feature] Site Migration Features related to site migrations to WPcom label Apr 15, 2024
@sixhours sixhours requested a review from a team April 15, 2024 17:53
@sixhours sixhours self-assigned this Apr 15, 2024
@sixhours sixhours requested a review from a team as a code owner April 15, 2024 17:53
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Apr 15, 2024
@gabrielcaires
Copy link
Contributor

@sixhours What is the benefit of repeating the same value we are already showing on the input?

@matticbot
Copy link
Contributor

matticbot commented Apr 15, 2024

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

@sixhours
Copy link
Contributor Author

@sixhours What is the benefit of repeating the same value we are already showing on the input?

@andres-blanco mentioned it in a comment on that thread:

It doesn’t mention the (invalid) URL you entered.

My thinking was the repetition might help them see the error in their URL.

But maybe we just need to not show the error message when the urlValue is empty?

@gabrielcaires
Copy link
Contributor

gabrielcaires commented Apr 15, 2024

But maybe we just need to not show the error message when the urlValue is empty?

Agree 👍

My thinking was the repetition might help them see the error in their URL.

Based on the data we have gathered, it seems that users are having trouble understanding the meaning of URL.
We have an issue just to improve the screen copy. #89543

@sixhours sixhours added the [Status] String Freeze Add the [Status] String Freeze label to your PR to ensure new strings are translated before merging label Apr 15, 2024
@sixhours sixhours changed the title Site Migration: Update error message to include the invalid URL Site Migration: Update form label for clarity Apr 15, 2024
@sixhours sixhours force-pushed the update/error-messages branch 2 times, most recently from 38e0bfd to c92b501 Compare April 15, 2024 20:45
@gabrielcaires
Copy link
Contributor

@sixhours What do you think about maintaining this PR by improving the error message and the other one updating the overall copy? My concern is we probably will have a revision that can make it hard to keep track of multiple PRs.

Regarding the error message, considering the high % of users setting wrong values as a domain.
What do you think about introducing an error message specific to users that set any value without a TLD?

Something like,
"Hey, I think you missed the domain suffix, they usually are (.com, .org, etc...)."
Just an example, I don't think it is a good copy :D

@sixhours sixhours removed the [Status] String Freeze Add the [Status] String Freeze label to your PR to ensure new strings are translated before merging label Apr 16, 2024
@matticbot
Copy link
Contributor

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

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

  • wpcom-block-editor

To test WordPress.com changes, run install-plugin.sh $pluginSlug update/error-messages on your sandbox.

@sixhours sixhours changed the title Site Migration: Update form label for clarity Site Migration: Don't show error message when urlValue is blank Apr 16, 2024
@sixhours
Copy link
Contributor Author

This change will cause issues because sometimes we show an error message after validating the URL and we preserve the urlValue in the text input, other times we'll validate the URL and clear the urlValue from the text input.

That's how you can end up in a state where the screen shows an error message but not the URL you submitted because they've entered an invalid URL like something.aisdfua.

Screenshot 2024-04-16 at 3 17 07 PM

I've been trying to figure out how to preserve the urlValue when it is invalid so we can offer the user more context.

@sixhours sixhours added [Status] In Progress and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Apr 16, 2024
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Apr 16, 2024
@sixhours sixhours changed the title Site Migration: Don't show error message when urlValue is blank Site Migration: Preserve urlValue in text input when an invalid URL is submitted Apr 16, 2024
@gabrielcaires
Copy link
Contributor

This change will cause issues because sometimes we show an error message after validating the URL and we preserve the urlValue in the text input, other times we'll validate the URL and clear the urlValue from the text input.

Thanks for sharing it, I can take a look while you are in AFK.

@gabrielcaires gabrielcaires self-assigned this Apr 17, 2024
@gabrielcaires gabrielcaires self-requested a review April 17, 2024 09:03
@gabrielcaires
Copy link
Contributor

gabrielcaires commented Apr 18, 2024

I think the form needs to be refactored to reduce the # of internal states.
setState is async and since we have multiple setState happening, that is probably causing race conditions.
We probably should do it only after merging the #89548

@@ -95,7 +95,7 @@ const CaptureInput: FunctionComponent< Props > = ( props ) => {
<FormTextInput
id="capture-site-url"
type="text"
className={ classnames( { 'is-error': showValidationMsg } ) }
className={ classnames( { 'is-error': showValidationMsg && urlValue } ) }
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a showValidationMsg field; why is this additional condition not placed there since it is dedicated to showing/hiding validation messages?

@gabrielcaires gabrielcaires removed their assignment May 13, 2024
@sixhours sixhours removed their assignment May 30, 2024
@sixhours
Copy link
Contributor Author

sixhours commented Jun 5, 2024

This is well out of date and we seem to have conflicting thoughts on how to address the original issue around lack of clarity, so I'm going to close this and create a new issue.

@sixhours sixhours closed this Jun 5, 2024
@github-actions github-actions bot removed [Status] In Progress [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Site Migration Features related to site migrations to WPcom
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants