Focused Launch: fix disabled free plan regressions, add missing "translators" comments #49523

ciampo · 2021-02-01T09:27:51Z

Changes proposed in this Pull Request

Use the disabledLabel when the plan's item is disabled
Change text color of the "custom domain" feature to orange, when custom domain is not available
Add missing "translators" comment for a string in the domain picker package
Format "translators" comments for a few launch-related strings, so that the comment is just above to the translated text (and not the sprintf function)

Testing instructions

Focused Launch

Prepare your sandbox:
- Make sure you have an active ssh connection to your sandbox
- Clear your sandbox and pull the latest version from trunk
- Add an unlaunched site created via /start to your sandbox
Pull this PR's branch on your local machine and run the project locally:
- Run yarn && yarn start in the root folder
- After the command above has finished compiling, in a separate terminal window run cd apps/editing-toolkit && yarn dev --sync
Visit calyspo.localhost:3000/page/UNLAUNCHED_SITE_CREATED_WITH_START.wordpress.com/home?flags=create/focused-launch-flow
Click on the "Launch" button to open Focused Launch flow
Pick a custom domain in the summary view
Click on "View all plans" to enter plan details view
- The free plan's button should be disabled, and the button's label should say "Unavailable with domain"
- The custom domain feature item's text color should be orange, same as the cross icon to its left
- The custom domain feature item's text-decoration prop should be strike-through.

Gutenboarding

Visit /new
Pick a custom domain in the domain picker, advance to the plans step
In the plans step, expand the free plan card, and not how the text/style of the feature hasn't changed from what's currently live in production

Screenshots

Before:

After:

Fixes #49409
Fixes #49525
Related to #49312

matticbot · 2021-02-01T09:27:53Z

Test live: https://calypso.live/?branch=fix/translators-comments

matticbot · 2021-02-01T09:38:02Z

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

App Entrypoints (~14 bytes added 📈 [gzipped])

name                 parsed_size           gzip_size
entry-gutenboarding        +44 B  (+0.0%)      +14 B  (+0.0%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

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 · 2021-02-01T10:38:04Z

Caution: This PR affects files in the Editing Toolkit Plugin on WordPress.com
Please ensure your changes work on WordPress.com before merging.

D56271-code has been created so you can easily test it on your sandbox. See this FieldGuide page about developing the Editing Toolkit Plugin for more info: PCYsg-ly5-p2

StefanNieuwenhuis · 2021-02-04T10:33:11Z

Gutenboarding testing instructions

Pick a custom domain in the domain picker, advance to the plans step

In the plans step, expand the free plan card, and not how the text/style of the feature hasn't changed from what's currently live in production

After picking a custom domain, I'm unable to expand the free plan since it's disabled. Am I following the instructions correctly or do you need to update them?

StefanNieuwenhuis · 2021-02-04T10:36:05Z

Not breaking, but what I noticed is that the X is aligned to the bottom. Shouldn't it be vertically aligned to the middle?

StefanNieuwenhuis

I left a few comments. When you have time, can you please address them? Thank you so much!

packages/plans-grid/src/plans-feature-list/style.scss

ciampo · 2021-02-04T13:09:14Z

Not breaking, but what I noticed is that the X is aligned to the bottom. Shouldn't it be vertically aligned to the middle?

The way the X is aligned made sense especially for the "accordion" variation of the plans grid.

I've changed the code so that the icon is vertically centered, but only in the table variant of the grid (the one used by focused launch flow). I'm not sure it looks better, I'm tempted to revert it to how it used to be (bottom aligned). What do you think?

After picking a custom domain, I'm unable to expand the free plan since it's disabled. Am I following the instructions correctly or do you need to update them?

Are you sure you are checking in Gutenboarding (/new) and not in Step by Step Launch? In Gutenboarding the free plan should never be disabled.

StefanNieuwenhuis · 2021-02-04T13:39:29Z

Are you sure you are checking in Gutenboarding (/new) and not in Step by Step Launch? In Gutenboarding the free plan should never be disabled.

Sorry, sorry, sorry (it's not my day today...) but I was checking step-by-step-launch indeed...Gutenboarding looks good!

StefanNieuwenhuis

Focused Launch

After selecting a custom (paid) domain and reviewing all plans on the plans grid, I noticed that the orange copy's text-decoration is none instead of line-through.

Screenshot

razvanpapadopol · 2021-02-05T08:46:41Z

packages/plans-grid/src/plans-table/plan-item.tsx

+															// translators: %s is a WordPress.com plan name (eg: Free, Personal)
+															__( 'Select %s', __i18n_text_domain__ ),
+															name
+													  ) }


I'm not sure if this is the same case as p1612463769120800/1612255293.049500-slack-C013QHLF28Y but worth checking.
cc: @yansern

I can rewrite it in a more "explicit" way for peace of mind

Done in 76c8e35

ciampo · 2021-02-05T11:25:26Z

Focused Launch

After selecting a custom (paid) domain and reviewing all plans on the plans grid, I noticed that the orange copy's text-decoration is none instead of line-through.

Screenshot

Hey @StefanNieuwenhuis , I've added the missing CSS selector in 5d99d61.

Would you mind giving it another look?

Move translator comments directly on top of `__()` function

…ns grid

…tent

…om domain text

StefanNieuwenhuis · 2021-02-05T13:52:24Z

Hey @StefanNieuwenhuis , I've added the missing CSS selector in 5d99d61.

Would you mind giving it another look?

Have checked it and it it all looks good now.

StefanNieuwenhuis

LGTM! 🚢

ciampo marked this pull request as draft February 1, 2021 09:38

ciampo force-pushed the fix/translators-comments branch from 5cd8d4a to 679c472 Compare February 1, 2021 10:28

ciampo marked this pull request as ready for review February 1, 2021 11:09

ciampo requested a review from a team February 1, 2021 11:09

matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Feb 1, 2021

ciampo self-assigned this Feb 1, 2021

ciampo added the Focused Launch Issues and PRs related to Focused Launch label Feb 1, 2021

ciampo added this to Needs review in Focused Launch (inactive) via automation Feb 1, 2021

ciampo changed the title ~~Fix/translators comments~~ Focused Launch: fix disabled free plan regressions, add missing "translators" comments Feb 3, 2021

StefanNieuwenhuis self-requested a review February 4, 2021 10:18

StefanNieuwenhuis reviewed Feb 4, 2021

View reviewed changes

packages/plans-grid/src/plans-feature-list/style.scss Outdated Show resolved Hide resolved

ciampo requested a review from StefanNieuwenhuis February 4, 2021 15:14

StefanNieuwenhuis requested changes Feb 4, 2021

View reviewed changes

razvanpapadopol reviewed Feb 5, 2021

View reviewed changes

ciampo requested review from StefanNieuwenhuis and razvanpapadopol February 5, 2021 11:25

ciampo added 6 commits February 5, 2021 14:47

Re-add "dsiabledLabel" back to plan items (table variant)

b1b6daa

Move translator comments directly on top of `__()` function

Add missing translator comment

670e4d7

Make text of disabled plan's domain feature orange and bold

caae4a0

Vertically center feature list item's icon for table variation of pla…

d661e7c

…ns grid

Refactor feature list styles, move styles closer to the HTML text con…

1488408

…tent

Add missing CSS selector to apply line-through style to disabled cust…

18f94db

…om domain text

Move translated text outside of ternary operator

81d2602

ciampo force-pushed the fix/translators-comments branch from 76c8e35 to 81d2602 Compare February 5, 2021 13:47

StefanNieuwenhuis approved these changes Feb 5, 2021

View reviewed changes

Focused Launch (inactive) automation moved this from Needs review to Ready to merge Feb 5, 2021

ciampo merged commit 3c064cf into trunk Feb 5, 2021

Focused Launch (inactive) automation moved this from Ready to merge to Done Feb 5, 2021

ciampo deleted the fix/translators-comments branch February 5, 2021 15:58

matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Feb 5, 2021

noahtallen mentioned this pull request Feb 6, 2021

ETK: update to v2.18 #49787

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Focused Launch: fix disabled free plan regressions, add missing "translators" comments #49523

Focused Launch: fix disabled free plan regressions, add missing "translators" comments #49523

ciampo commented Feb 1, 2021 •

edited

matticbot commented Feb 1, 2021

matticbot commented Feb 1, 2021 •

edited

matticbot commented Feb 1, 2021

StefanNieuwenhuis commented Feb 4, 2021

StefanNieuwenhuis commented Feb 4, 2021

StefanNieuwenhuis left a comment

ciampo commented Feb 4, 2021

StefanNieuwenhuis commented Feb 4, 2021

StefanNieuwenhuis left a comment •

edited

razvanpapadopol Feb 5, 2021

ciampo Feb 5, 2021

ciampo Feb 5, 2021

ciampo commented Feb 5, 2021

Focused Launch

StefanNieuwenhuis commented Feb 5, 2021

StefanNieuwenhuis left a comment

Focused Launch: fix disabled free plan regressions, add missing "translators" comments #49523

Focused Launch: fix disabled free plan regressions, add missing "translators" comments #49523

Conversation

ciampo commented Feb 1, 2021 • edited

Changes proposed in this Pull Request

Testing instructions

Focused Launch

Gutenboarding

Screenshots

matticbot commented Feb 1, 2021

matticbot commented Feb 1, 2021 • edited

matticbot commented Feb 1, 2021

StefanNieuwenhuis commented Feb 4, 2021

Gutenboarding testing instructions

StefanNieuwenhuis commented Feb 4, 2021

StefanNieuwenhuis left a comment

Choose a reason for hiding this comment

ciampo commented Feb 4, 2021

StefanNieuwenhuis commented Feb 4, 2021

StefanNieuwenhuis left a comment • edited

Choose a reason for hiding this comment

Focused Launch

razvanpapadopol Feb 5, 2021

Choose a reason for hiding this comment

ciampo Feb 5, 2021

Choose a reason for hiding this comment

ciampo Feb 5, 2021

Choose a reason for hiding this comment

ciampo commented Feb 5, 2021

Focused Launch

StefanNieuwenhuis commented Feb 5, 2021

StefanNieuwenhuis left a comment

Choose a reason for hiding this comment

ciampo commented Feb 1, 2021 •

edited

matticbot commented Feb 1, 2021 •

edited

StefanNieuwenhuis left a comment •

edited