-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Signup reskin: Integration PR #50512
Conversation
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~438 bytes added 📈 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~6544 bytes added 📈 [gzipped])
Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Async-loaded Components (~880 bytes removed 📉 [gzipped])
React components that are loaded lazily, when a certain part of UI is displayed for the first time. 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. Generated by performance advisor bot at iscalypsofastyet.com. |
1d8a546
to
204d1fb
Compare
e3d7a0b
to
f355092
Compare
@jessie-ross Since this is the first time I'm trying out the new ExPlat JS client, your inputs here would be super helpful. I also need help with a weird behaviour I'm seeing with getting the experiment assignment. If I'm pointed to sandbox, then the variation is fetched immediately. However, if I'm pointed to production, then the variation returns undefined first and then eventually returns the assignment. I'm able to consistently reproduce this behaviour. To repro, try the following steps:
If I'm pointed to sandbox, then /start will load the user step correctly with the white background. Notice in the console messages how the experiment variation is picked up as treatment(I logged the value of I'm pointed to production, then /start loads control experience first. Notice the console messages picking up undefined first and then treatment. I'm curious why there's a difference in behaviour? |
29ea9ee
to
75c94c0
Compare
Ah it is this error being thrown and causing experimentAssignment to be null: wp-calypso/packages/explat-client/src/create-explat-client.ts Lines 176 to 187 in ce5a924
It didn't occur in the sandbox since it took so long... Looking at it now perhaps throwing an error wasn't the right idea, I'm going to PR it right now changing it to logging and removing the need to wrap with a try catch... In the meantime you could use |
This has been fixed and merged in #51564, if you merge it in here you should find it works correctly 😁 |
35af23b
to
735e378
Compare
client/signup/index.web.js
Outdated
@@ -9,13 +9,10 @@ import page from 'page'; | |||
import controller from './controller'; | |||
import { makeLayout, render as clientRender } from 'calypso/controller'; | |||
import { getLanguageRouteParam } from 'calypso/lib/i18n-utils'; | |||
import { loadExperimentAssignment } from 'calypso/lib/explat'; | |||
|
|||
export default async function () { | |||
const lang = getLanguageRouteParam(); | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is a good idea to add a non-awaited loadExperimentAssignment('refined_skin_v1')
in here as well - this will prefetch the experiment while everything else is loading so there will be less chance of a loading state occuring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah good point, I'll do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've implemented this change. The PR is ready for review once again :)
LGTM from the ExPlat side of things (so long as manual testing it works well) 👍 I recommend using an Exposure event with this one to further target the FYI in the very near future (WIP PR #51565) we will be adding an <ProvideExperimentData
name="refined_reskin_v1"
options={{ isEligible: 'onboarding' === this.props.flowName }}
> This means you would get the default experience and no assignment would occur if |
Thanks for the quick review and feedback!
Makes sense. I haven't gotten around to defining the exposure events in Abacus yet but will keep this in mind.
This is going to be a very helpful change, thanks for working on it. |
ee8457a
to
42ba61d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works as described! 👍
* Setup experiment in ExPlat and prevent blue flicker for the treatment group * Remove blue flicker when the treatment loads signup user step * Use isReskinned props from parent * Remove flicker of old user step UI when the treatment loads the user step * Refactor * Implement the new ExPlat JS client * Format header and subheader in the reskin flow * Change shade of background white * Setup experiment in ExPlat and prevent blue flicker for the treatment group * Remove blue flicker when the treatment loads signup user step * Use isReskinned props from parent * Remove flicker of old user step UI when the treatment loads the user step * Refactor * Implement the new ExPlat JS client * Change shade of background white * Remove Experiment component that is now deprecated * Fix merge issues * Fix merge issues * Setup experiment in ExPlat and prevent blue flicker for the treatment group * Remove blue flicker when the treatment loads signup user step * Use isReskinned props from parent * Remove flicker of old user step UI when the treatment loads the user step * Refactor * Implement the new ExPlat JS client * Change shade of background white * Remove Experiment component that is now deprecated * Use the ProvideExperimentData component for fetching experiment assignment * 1. Remove commented out lines 2. Load experiment assignments while signup is loading * Remove commented line * Undo a few debugging changes * Remove a remnant from merge conflict * Hide the header in the mapping/transfer screen * Setup experiment in ExPlat and prevent blue flicker for the treatment group * Remove blue flicker when the treatment loads signup user step * Use isReskinned props from parent * Remove flicker of old user step UI when the treatment loads the user step * Refactor * Implement the new ExPlat JS client * Change shade of background white * Remove Experiment component that is now deprecated * Use the ProvideExperimentData component for fetching experiment assignment * 1. Remove commented out lines 2. Load experiment assignments while signup is loading * Remove commented line * Undo a few debugging changes * Setup experiment in ExPlat and prevent blue flicker for the treatment group * Remove blue flicker when the treatment loads signup user step * Use isReskinned props from parent * Remove flicker of old user step UI when the treatment loads the user step * Refactor * Implement the new ExPlat JS client * Change shade of background white * Remove Experiment component that is now deprecated * Use the ProvideExperimentData component for fetching experiment assignment * 1. Remove commented out lines 2. Load experiment assignments while signup is loading * Remove commented line * Undo a few debugging changes * CSS tweaks * Setup experiment in ExPlat and prevent blue flicker for the treatment group * Remove blue flicker when the treatment loads signup user step * Use isReskinned props from parent * Remove flicker of old user step UI when the treatment loads the user step * Refactor * Implement the new ExPlat JS client * Change shade of background white * Remove Experiment component that is now deprecated * Use the ProvideExperimentData component for fetching experiment assignment * 1. Remove commented out lines 2. Load experiment assignments while signup is loading * Remove commented line * Undo a few debugging changes * Setup experiment in ExPlat and prevent blue flicker for the treatment group * Remove blue flicker when the treatment loads signup user step * Use isReskinned props from parent * Remove flicker of old user step UI when the treatment loads the user step * Refactor * Implement the new ExPlat JS client * Change shade of background white * Remove Experiment component that is now deprecated * Use the ProvideExperimentData component for fetching experiment assignment * 1. Remove commented out lines 2. Load experiment assignments while signup is loading * Remove commented line * Undo a few debugging changes * Signup reskin: Add the side section for the domain step (#51499) * Add a side section to the domain step that has the free domain explainer and the use your domain link * Remove reskinned prop in freedomainexplainer component * Fix padding for the search input box * Disable eslint for click event error and fix a couple of eslint errors * Modify CSS to move the width restrictions further up the div hierarchy * Refactor: the strings for the sidebar are now stored in the component to make it more reusable * Some css tweaks * Signup reskin: Remove example suggestions and dropdown filters (#51504) * Remove example suggestions and dropdown filter * Add the tip icon; change the search icon color * CSS tweaks * CSS tweaks * New search icon for the input box * Fix error with SVG import * Move svg file to the main search component * Use the new search component * Signup reskin: Update UI for domain search results (#51524) * Modify UI for the featured domain results * More UI changes to the featured domains section * UI fixes to match with the Figma sketch * Change background color of show more tld filter * UI fix * Some alignment fixes * 1. Remove stray console log line 2. Show progress bar adjacent to the featured domain name only for the variant * Don't show the user your domain line in the search results, since we now show it in the sidebar * Wrap the div only for treatment, so that control remains unaffected * CSS tweaks * Refactor and consolidate * Signup reskin: Mobile tablet views (#51611) * WIP commit - mobile breakpoint CSS changes * Show the sidebar section below the tld filter bar for mobile view * More fixes for the mobile view * UI changes * More UI fixes for the mobile and tab views * Fix the subheader length * A few UI fixes * Show/hide the side bar content via CSS instead of using Calypso's viewport settings * Remove border bottom in iPad view * Fix header alignment * Show free domain explainer before search results are rendered in mobile view * CSS tweaks * CSS tweaks * Signup reskin: UI changes for the user step (#51648) * UI changes for the reskinned user step * Define disabled button colors * UI fix for the header section * Responsive web changed * CSS changes to tweak a few design related things; Refactored to bring all reskin related css changes in a single file * Fix alignment issues in smaller screen sizes * More CSS changes to bring the page closer to the design
…n mobile screen sizes in both control and treatment
7a473fc
to
e7874d3
Compare
} | ||
|
||
renderReskinDomainPrice() { | ||
const priceText = this.props.translate( '%(cost)s/year', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 17 times:
translate( '%(cost)s /year' )
ES Score: 7
…elds, fill width in user step
This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/5736615 Thank you @niranjan-uma-shankar for including a screenshot in the description! This is really helpful for our translators. |
Translation for this Pull Request has now been finished. |
Changes proposed in this Pull Request
This PR replaces the legacy experiments platform with the new ExPlat JS client for the reskinned signup experiment. Please note that the UI changes for this experiment are implemented in subsequent PRs. This PR only swaps out the legacy experiment platform for the new one.
For more context on the experiment, read pbxNRc-IO-p2.
Testing instructions