Signup: Fix Themes step horizontal scroll #11299

bisko · 2017-02-09T16:54:01Z

This PR is a fix for #8745.

The Pressable "substeps" in Signup break the design a bit by being shifted to the side. Even though they are hidden, they still expand the bounding box and cause the scroll.

I'm trying to make the substeps play nice with the main steps and not cause such problems.

To test:

Start Signup
Reach Themes step
Check if there is horizontal scroll - there shouldn't be
Check if the Upload theme option is working correctly
Check the Themes step submits properly
Finish Signup and verify that the resulting site has the chosen theme
Verify no JS errors appear in the console

Note:
After starting signup, you need to get yourself in the Theme Upload AB test, as @lsinger mentioned below:

FWIW, to activate the Upload Theme option:

localStorage.setItem('ABTests', '{"signupThemeUpload_20160928": "showThemeUpload"}')

matticbot · 2017-02-09T16:54:08Z

Test live: https://calypso.live/?branch=fix/signup-themes-step-pressable-overlay-no-scroll

lsinger · 2017-02-09T21:15:14Z

FWIW, to activate the Upload Theme option:

localStorage.setItem('ABTests', '{"signupThemeUpload_20160928": "showThemeUpload"}')

lsinger · 2017-02-09T21:41:44Z

client/signup/steps/theme-selection/index.jsx

-			'theme-selection__themes-wrapper': true,
-			'is-hidden': this.state.showPressable,
-		} );
+		let headerText = translate( 'Choose a theme.' );


I think we'd prefer to avoid the use of let in favor of const if possible.

In this case:

const headerText = this.state.showPressable ? "...?" : "...!"; // etc.

I did it with the shorthand if syntax and const, but didn't look very nice and wasn't very obvious what was happening there:

const headerText = this.state.showPressable ? translate( 'Upload your WordPress Theme' ) : translate( 'Choose a theme.' ); const subHeaderText = this.state.showPressable ? translate( 'Our partner Pressable is here to help you' ) : translate( 'No need to overthink it. You can always switch to a different theme later.' );

versus:

let headerText = translate( 'Choose a theme.' ); let subHeaderText = translate( 'No need to overthink it. You can always switch to a different theme later.' ); if ( this.state.showPressable ) { headerText = translate( 'Upload your WordPress Theme' ); subHeaderText = translate( 'Our partner Pressable is here to help you' ); }

I think the second one is better for readability if you're quickly skimming through the code and I don't think let/const would make a huge difference in terms of performance here :)

lsinger

I did get an error in the console on the page where I had to enter the user details:

vendor.development.js?v=9278c344b7:3350 Warning: `value` prop on `input` should not be null. Consider using the empty string to clear the component or `undefined` for uncontrolled components.
    in input (created by FormTextInput)
    in FormTextInput (created by SignupForm)
    in fieldset (created by FormFieldset)
    in FormFieldset (created by ValidationFieldset)
    in ValidationFieldset (created by SignupForm)
    in div (created by SignupForm)
    in form (created by LoggedOutForm)
    in div (created by Card)
    in Card (created by LoggedOutForm)
    in LoggedOutForm (created by SignupForm)
    in div (created by SignupForm)
    in SignupForm (created by UserStep)
    in div (created by StepWrapper)
    in div (created by StepWrapper)
    in StepWrapper (created by UserStep)
    in UserStep (created by LocalizedUserStep)
    in LocalizedUserStep (created by Connect(LocalizedUserStep))
    in Connect(LocalizedUserStep) (created by Signup)
    in div (created by Signup)
    in ReactCSSTransitionGroupChild (created by ReactTransitionGroup)
    in span (created by ReactTransitionGroup)
    in ReactTransitionGroup (created by ReactCSSTransitionGroup)
    in ReactCSSTransitionGroup (created by Signup)
    in span (created by Signup)
    in Signup (created by Connect(Signup))
    in Connect(Signup)
    in Provider

And then, when submitting the form:

vendor.development.js?v=9278c344b7:3350 Warning: FormTextInput is changing an uncontrolled input of type email to be controlled. Input elements should not switch from uncontrolled to controlled (or vice versa). Decide between using a controlled or uncontrolled input element for the lifetime of the component. More info: https://fb.me/react-controlled-components

vendor.development.js?v=9278c344b7:3350 Warning: Failed prop type: Invalid prop `user` of type `boolean` supplied to `SignupProcessingScreen`, expected `object`.
    in SignupProcessingScreen (created by Signup)
    in Signup (created by Connect(Signup))
    in Connect(Signup)
    in Provider

vendor.development.js?v=9278c344b7:3350 Warning: setState(...): Can only update a mounted or mounting component. This usually means you called setState() on an unmounted component. This is a no-op. Please check the code for the SignupForm component.

And then, on the front-end after seeing Trampoline:

bootstrap 43c1d8f…:9 Uncaught TypeError: Cannot read property 'type' of undefinedh @ bootstrap 43c1d8f…:9(anonymous function) @ bootstrap 43c1d8f…:9(anonymous function) @ bootstrap 43c1d8f…:9n @ bootstrap 43c1d8f…:16h @ bootstrap 43c1d8f…:16d @ bootstrap 43c1d8f…:16

Then again I also get all of these on master. This PR doesn't seem to add any problems.

I confirm there's no horizontal scroll bar in the themes step. Clicking "Upload Theme" works, and going back as well.

Apart from the one comment I left regarding let vs. const: LGTM! 🚢

lsinger · 2017-02-10T11:02:03Z

Thanks for the update! 👍

mcsf · 2017-02-10T10:58:52Z

client/signup/step-wrapper/index.jsx

@@ -14,7 +14,14 @@ import config from 'config';
 export default React.createClass( {
 	displayName: 'StepWrapper',

+	propTypes: {
+		hideNavButtons: React.PropTypes.bool


Drive-by comment: shouldHideNav[Buttons]

mcsf · 2017-02-10T11:00:19Z

client/signup/steps/theme-selection/index.jsx

@@ -66,13 +66,34 @@ class ThemeSelectionStep extends Component {
 	};

 	renderThemesList() {
+		const pressableWrapperClassName = classNames( {
+			'theme-selection__pressable-wrapper': true,


If this key is always true I think classnames allows single strings as arguments. Also, check which is more common in calypso: classnames or classNames

344 classNames( 106 classnames(

mcsf · 2017-02-10T11:00:38Z

client/signup/steps/theme-selection/index.jsx

+		} );
+
+		const themesWrapperClassName = classNames( {
+			'theme-selection__themes-wrapper': true,


Same applies here

mcsf · 2017-02-10T11:01:22Z

client/signup/steps/theme-selection/index.jsx

+			<div className="theme-selection__pressable-substep-wrapper">
+				<div className={ pressableWrapperClassName }>
+					<PressableThemeStep
+						{ ... this.props }


Remove space after ellipsis

mcsf · 2017-02-10T11:02:56Z

client/signup/steps/theme-selection/index.jsx

-		const { translate } = this.props;
+		const subHeaderText = this.state.showPressable
+			? translate( 'Our partner Pressable is here to help you' )
+			: translate( 'No need to overthink it. You can always switch to a different theme later.' );


Maybe add context for translators. The other strings may not need it, but this one probably does.

a8ci18n · 2017-02-10T11:59:17Z

client/signup/steps/theme-selection/index.jsx

+			? translate( 'Our partner Pressable is here to help you', {
+				context: 'Subheader text in Signup, when a user chooses the Upload theme in the Themes step'
+			} )
+			: translate( 'No need to overthink it. You can always switch to a different theme later.', {


Hi! I've found a possible matching string that has already been translated 22 times:
translate( 'No need to overthink it. You can always switch to a different theme later.' ) ES Score: 14.48
See 2 additional suggestions in the PR translation status page

Help me improve these suggestions: react with 👎 if the suggestion doesn't make any sense, or with 👍 if it's a particularly good one (even if not implemented).

a8ci18n · 2017-02-10T11:59:18Z

client/signup/steps/theme-selection/index.jsx


-		const { translate } = this.props;
+		const subHeaderText = this.state.showPressable
+			? translate( 'Our partner Pressable is here to help you', {


Hi! I've found a possible matching string that has already been translated 18 times:
translate( 'Our partner Pressable is here to help you' ) ES Score: 13.91
See 1 additional suggestion in the PR translation status page

Help me improve these suggestions: react with 👎 if the suggestion doesn't make any sense, or with 👍 if it's a particularly good one (even if not implemented).

Fix Themes step horizontal scroll

123711e

bisko added [Feature] Signup & Account Creation All screens and flows for making a new WordPress.com account. [Type] Bug labels Feb 9, 2017

bisko self-assigned this Feb 9, 2017

bisko requested review from lsinger, mcsf, marekhrabe and michaeldcain February 9, 2017 16:54

Fix leftover horizontal scroll

6b28615

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

lsinger reviewed Feb 9, 2017

View reviewed changes

lsinger approved these changes Feb 9, 2017

View reviewed changes

Update the way header texts are set

81c51c1

lsinger added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Feb 10, 2017

mcsf reviewed Feb 10, 2017

View reviewed changes

Update code according to code review

3fab71e

a8ci18n reviewed Feb 10, 2017

View reviewed changes

bisko merged commit e078ace into master Feb 10, 2017

matticbot removed the [Status] Ready to Merge label Feb 10, 2017

bisko deleted the fix/signup-themes-step-pressable-overlay-no-scroll branch February 10, 2017 15:48

This was referenced Feb 10, 2017

Signup: Theme step has horizontal scrollbar in Firefox #8745

Closed

Signup: Fix horizontal scroll on Design Type With Store Signup step #11331

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Signup: Fix Themes step horizontal scroll #11299

Signup: Fix Themes step horizontal scroll #11299

bisko commented Feb 9, 2017 •

edited

Loading

matticbot commented Feb 9, 2017

lsinger commented Feb 9, 2017

lsinger Feb 9, 2017

bisko Feb 10, 2017

lsinger left a comment

lsinger commented Feb 10, 2017

mcsf Feb 10, 2017

mcsf Feb 10, 2017

bisko Feb 10, 2017 •

edited

Loading

mcsf Feb 10, 2017

mcsf Feb 10, 2017

mcsf Feb 10, 2017

a8ci18n Feb 10, 2017

a8ci18n Feb 10, 2017

Signup: Fix Themes step horizontal scroll #11299

Signup: Fix Themes step horizontal scroll #11299

Conversation

bisko commented Feb 9, 2017 • edited Loading

matticbot commented Feb 9, 2017

lsinger commented Feb 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lsinger left a comment

Choose a reason for hiding this comment

lsinger commented Feb 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bisko Feb 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bisko commented Feb 9, 2017 •

edited

Loading

bisko Feb 10, 2017 •

edited

Loading