-
Notifications
You must be signed in to change notification settings - Fork 2k
Use core button component for continue as user screens #103097
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
Use core button component for continue as user screens #103097
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
|
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
|
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~41 bytes added 📈 [gzipped]) DetailsCommon code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~12 bytes added 📈 [gzipped]) DetailsSections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Async-loaded Components (~12 bytes added 📈 [gzipped]) DetailsReact 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. |
33556b8 to
77f410d
Compare
b111fa8 to
b42a06f
Compare
1b39c5e to
3ce9377
Compare
f842b0c to
812efc3
Compare
c431834 to
f01e4c2
Compare
3e9fcff to
38b985c
Compare
| &-right-element { | ||
| margin-left: auto; | ||
| --wp-components-color-accent: var( --color-neutral-100 ); | ||
| --color-accent: var( --color-neutral-100 ); |
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.
This element uses a core link button and is blue with the new theming for these without this.
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.
CC: @ciampo @youknowriad for input 🙏
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 you explain the issue a bit better?
I can see you're linking to some style overrides that are changing an internal variable used for text color. Why did we need those overrides in the first place, and why do we need to override those overrides? We should use the original Button specs and pick the right variant via the variant prop
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.
Sure 🙂
The color overrides for buttons were added in #101279, and this PR expands that set to enable theming of link variants as well (which this button is). This instance of the link button is not themed though (always grey color, not the product accent color) so we need to override the accent color, which changes --wp-components-color-accent .
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 see how we're trying to match the "before" design with the black text for the "Create an account" button, but it's unclear to me why it justifies a design system exception here. There are three different styles of link-like elements in this simple view. Can the "Create an account" button just use the standard accent color styling? @jasmussen
Because if this is justified, that seems like it will be a common need, and we will need to add it to the system.
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.
Thanks all.
The top right button on http://calypso.localhost:3000/setup/onboarding/playground is now
--color-neutral-100and no longer the accent color. Is this intentional? Should we change something here? Thanks.
No need to do anything, I'll remove this override and let the accent color shine through 🙂
if we absolutely need some overrides, they should be project-wide and live in client/assets/stylesheets/_wp-components-overrides.scss
The overrides in that file for buttons are our mechanism for applying branding per product. I can look to move all the instances where we set the button accent color for a product into this file. I agree centralizing these makes sense.
(although we should ideally remove those too)
I'm not sure how we would apply this branding if we remove these, can you suggest a better solution?
if in need of applying styles to button (not overrides), we should not use .components-button — instead, the Button should be assigned a className, and that classname should be used to add the styles;
I'll take a look at this. There is a bit of a mixture at the moment. For some brands, eg. Woo, where all the buttons share similar styles I've styled the components-button.
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.
PR for removing the override #103610
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.
PR for centralizing overrides for branding #103611
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 left a comment on the PR that's related to that color intentionally being neutral. But it's not a strong opinion, it's more a matter of us needing to get some quick brand input.
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'm not sure how we would apply this branding if we remove these, can you suggest a better solution?
We're working on a Theme package, which should allow us to theme our components (including applying a brand color). We can keep the centralised overrides until components are Themed
6277d47 to
8ea3fdc
Compare
| return null; | ||
| } | ||
|
|
||
| if ( this.props.currentUser && ! this.props.disableContinueAsUser ) { |
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.
Moving this before the next condition ensures it's displayed for Crowdsignal too
| 'a8c-for-agencies': isA4AOAuth2Client( this.props.oauth2Client ), | ||
| crowdsignal: isCrowdsignalOAuth2Client( this.props.oauth2Client ), |
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.
Add classes to hook into existing styles
| 'is-unified-site-sidebar-visible': this.props.isUnifiedSiteSidebarVisible, | ||
| 'is-blaze-pro': this.props.isBlazePro, | ||
| 'is-woo-com-oauth': isWooOAuth2Client( this.props.oauth2Client ), | ||
| 'jetpack-cloud': isJetpackCloudOAuth2Client( this.props.oauth2Client ), |
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.
Add class to hook into existing styles
| align-self: stretch; | ||
| border: 1px solid #ccc; | ||
| height: 221px; | ||
| width: 100%; |
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.
Remove unnecessary fixed height and make card fill flex column width
| --color-link: var(--studio-jetpack-green-40); | ||
| --color-link-dark: var(--studio-jetpack-green-60); |
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.
Use theming to change link color instead of anchor styles below, which clash with core button link styles
tellthemachines
left a comment
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 managed to test most of these, except for wp cloud, a4a and partner portal which on login take me to the public API URL, and that doesn't translate to Calypso. Not sure what's going on there 😕
The code looks good! Mostly questions below.
| busy={ validatingPath } | ||
| isBusy={ validatingPath } | ||
| href={ validatedPath || '/' } | ||
| __next40pxDefaultSize |
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.
Woo and Blaze Pro sections are incredibly similar; maybe an opportunity to consolidate in a later PR?
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.
Agreed, ideally these would all just use the white login screen
Yeah it's a bit of a dance. I log into sandbox/prod, then use browser navigation to get back to the login screen and then click the 'create an account' link below the primary button: Resulting screen and URL should be like this: Then you can change the host. |
|
Ok so clicking back once on the "Welcome to Automattic Partner Portal" screen takes me back to the wp.com continue as user screen, but there's nothing before that point in browser history. Clicking back again takes me to an empty tab 😕 I started from the hosts URL that immediately directed me to |
Oh yeah I see what you mean, with a fresh tab I get the same. Let me try to find another set of steps. |
It does seem hard to get to this variant in prod for the brands I can't add login links for, but I can get to it in dev with Crowdsignal.
And I can do the same for the other brands when I have the localhost login screen open already |
tellthemachines
left a comment
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.
Thanks for the extra instructions, I've now managed to test the remaining screens. LGTM!
Thanks for persisting 😁 |
3212bd8 to
d915c91
Compare
d915c91 to
936825f
Compare










Related to DSGCOM-92: Update primary buttons on 'Continue as user'
Proposed Changes
Why are these changes being made?
These buttons have no visible focus styles, and are inconsistent with the buttons on the login, magic login and signup screens, which have all been replaced.
Testing Instructions
Instructions
Login links
WordPress.com
Crowdsignal
Jetpack Cloud
Woo
Blaze Pro
Akismet
A4A - Can't link due to GitHub OSS scan rules: go to automatt_c.com/for-agencies/ and click 'Log in'. Edit the URL and change the host to calypso.localhost:3000
VIP
Partner Portal - Can't link due to GitHub OSS scan rules: go to hosts.automatt_c.com and you should be redirected to log in. Edit the URL and change the host to calypso.localhost:3000
WP.Cloud - Can't link due to GitHub OSS scan rules: go to wp.cloud and click 'start here'. Edit the URL and change the host to calypso.localhost:3000
Screenshots (showing buttons focused on desktop)
Pre-merge Checklist