-
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
P2: signup: initial styling #43021
P2: signup: initial styling #43021
Conversation
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Sections (~311 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 (~105 bytes added 📈 [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. |
85bdbe9
to
9cbd7b7
Compare
62deddf
to
c3cd652
Compare
9cbd7b7
to
e721387
Compare
b28fbad
to
06b587e
Compare
I still need to add translations and unify the css to variables, but it's ready for testing and feedback! |
Tests great! @evilluendas you can use https://hash-acc9b837d2f8b890fc904fe0947d13aab31fed76.calypso.live/start/p2/p2-site This feels too static while waiting |
@griffbrad hey, could you give this a look too, please? :) |
7eb608a
to
d1c33ea
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.
Overall, LGTM. Left a few comments about minor JS API changes you could make, but I realize you were mostly mirroring surrounding usages already present. On the simpler, purely presentation components (e.g. P2StepWrapper
) it probably also makes sense to use a function component rather than a class component with just a render
method.
client/signup/controller.js
Outdated
return; | ||
} | ||
|
||
document.body.className = document.body.className.split( 'is-p2-signup' ).join( '' ); |
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.
Could use classList here:
https://developer.mozilla.org/en-US/docs/Web/API/Element/classList
client/signup/controller.js
Outdated
@@ -70,6 +88,18 @@ export default { | |||
context.params.flowName === 'account' | |||
) { | |||
removeWhiteBackground(); | |||
next(); | |||
} else if ( context.pathname.indexOf( 'p2' ) >= 0 ) { |
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.
includes
could be used here:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/includes
<div className="p2-processing-screen"> | ||
<div className="p2-processing-screen__logo"> | ||
<img | ||
src="https://wpcom.files.wordpress.com/2020/06/p2-logo-light.png" |
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.
Should probably add this to static assets in Calypso:
https://github.com/Automattic/wp-calypso/tree/master/static/images
Thank you for the great feedback @griffbrad! Addressed. |
99e3ac5
to
a6fb114
Compare
@@ -253,7 +251,7 @@ class P2Site extends React.Component { | |||
className="p2-site__validation-site-title" | |||
> | |||
<FormLabel htmlFor="site-title-input"> | |||
{ this.props.translate( "What's the name of your team or project?" ) } | |||
{ this.props.translate( 'Name your team or project' ) } |
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 20 times:
translate( 'Please enter your team or project name.' )
ES Score: 7
I just pushed a few tweaks to the css, mostly overwriting the Calypso styles in the form inputs. I'd like to tweak a couple of things so it looks better in mobile, I'll do it asap and commit the changes. I've noticed a couple of things that I don't know how to fix:
Aside from that, looks great! I'd like to add some animation to the background of the "Hooray, site is creating" screen but I'd need to pause the flow there instead of automatically go to the frontend and I'll need your help, as we've talked in Slack. 😄 We'll also need to implement the second step explaining (461-gh-p2) that P2 works with wpcom accounts, but we can do it in a separate PR. |
8124f75
to
66956ab
Compare
Thanks for the styling fixes @evilluendas and sorry for missing those. I'll look at the rest of the things you've mentioned. |
fb93191
to
ef35d10
Compare
131c94c
to
c8e2757
Compare
This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/3857978 Hi @lamosty, could you please edit the description of this PR and add a screenshot for our translators? Ideally it'd include all of the following strings:
Thank you in advance! |
Translation for this Pull Request has now been finished. |
Part of introducing the revamped signup flow for the P2 project.
In this PR, we add the redesigned "site" signup step for P2.
Testing instructions
Navigate to
http://calypso.localhost:3000/start/p2
and go through the flow. It should look and work fine. Try both in an incognito window (=> new wp.com account) and with your current wp.com account.