-
Notifications
You must be signed in to change notification settings - Fork 2k
Implement yoast domain addition step behind feature flag #52081
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
Conversation
|
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~1457 bytes added 📈 [gzipped]) DetailsCommon code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~16335 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 (~53148 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. |
990136a to
0e1fcef
Compare
12475ea to
158efe6
Compare
7da0838 to
eaecc4d
Compare
| stroke="#1D2327" | ||
| strokeWidth="1.5" | ||
| /> | ||
| </svg> |
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.
Let's use the standardized arrow-left gridicon here instead of drawing your own arrow.
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.
We could also try using Gutenberg's dashicons - last time I checked there were 3 possible arrow variations we could use here.
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.
Fixed by 0aa0786 I used @wordpress/icons
| ...selectedDomain, | ||
| }; | ||
| addProductsToCart( [ domainProduct ] ); | ||
| window.location.href = window.location.origin + '/checkout/' + selectedSite.slug; |
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.
A full page reload should not be needed here. And that's what window.location does. Let's do a normal SPA navigation with page() here.
Also, other places that addProductsToCart wait until the adding is done before redirecting:
addProductsToCart( [ domainProduct ] ).then( () => {
page( '/checkout/' + selectedSite.slug );
} );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.
Fixed by 0aa0786
| <div className="marketplace-domain-upsell__header domains__header"> | ||
| <h1 className="marketplace-domain-upsell__header marketplace-title">Choose a domain</h1> | ||
| <h2 className="marketplace-domain-upsell__header marketplace-subtitle"> | ||
| Yoast SEO requires a top level domain to index your site and help you rank higher. |
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.
The copy should say "custom domain" instead of "top level domain". The term "top level domain" refers to actual TLDs like .com or .blog.
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.
Is then wpataging.com a custom domain?
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.
*.wpcomstaging.com or *.wordpress.com are not considered custom domains -- only actual unique registered domains are. I don't know what exactly the technical requirements of Yoast SEO are, but "top level domain" is almost certainly technically incorrect here.
| } | ||
| } | ||
| .marketplace-title { | ||
| font-family: var( --p2-font-inter ); |
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.
Why does the page use this custom font? Is there a reason why to not use Calypso defaults?
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.
Fixed by 0aa0786
| font-family: var( --p2-font-inter ); | ||
| font-style: normal; | ||
| letter-spacing: 0.4px; | ||
| color: #1d2327; |
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.
Using hardcoded custom colors is suspicious: let's use appropriate predefined colors (with CSS variables) that can be modified by a custom Calypso theme.
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.
Fixed by 0aa0786
|
My notes:
|
c1aac3e to
52901e2
Compare
| } | ||
|
|
||
| export function renderDomainsPage( context, next ) { | ||
| context.primary = <MarketplaceDomainUpsell />; |
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.
Seems like this is inflating the plugins section by a huge percent (38.3% gzipped according to ICFY). Is this something we can try to AsyncLoad? Or maybe we can AsyncLoad pieces in it that are responsible for the inflation, like the domain picker for example?
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import DomainPicker from '@automattic/domain-picker'; |
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.
Nit: Can we move all those @automattic/ to the External dependencies section above? While they're internal for the repo, they're actually external for the package we're using them in (client/package.json AKA calypso).
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.
Fixed by 0aa0786
| import { getSelectedSite } from 'calypso/state/ui/selectors'; | ||
|
|
||
| import './styles.scss'; | ||
| import { getProductsList, isProductsListFetching } from 'calypso/state/products-list/selectors'; |
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.
Nit: This seems to belong above, with the rest of the internal imports.
Also: I've seen styles being imported with this doc block above them, in case this is something you'd like to use:
/**
* Style dependencies
*/
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.
Fixed by 0aa0786
| import { Button } from '@wordpress/components'; | ||
| import { getSelectedSite } from 'calypso/state/ui/selectors'; | ||
|
|
||
| import './styles.scss'; |
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.
Nit: it's much more common to name this style.scss instead of styles.scss. Although both can be found throughout the codebase. Would be nice if we had just 1 way to do those things 😉
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.
Fixed by 0aa0786
client/my-sites/plugins/marketplace/marketplace-domain-upsell/index.tsx
Outdated
Show resolved
Hide resolved
| </div> | ||
| <hr /> | ||
| <h1 className="marketplace-domain-upsell__shopping-cart total"> | ||
| <div> Total </div> |
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.
<div />s inside <h1 />s?
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.
Fixed by 0aa0786
| ); | ||
| } ) } | ||
| </div> | ||
| <hr /> |
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 we achieve this by using styles or dedicated components?
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.
Can you elaborate on this @tyxla, do we have any dedicated customizable components for <hr /> ?
Also do you mean to add a new styled component here for hr? Is there any additional advantage to add a styled component here? Really liked to know. Are we switching to a convention of using styled components across calypso?
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 we're talking about different things here.
What I meant was that we could use a different component instead of a raw <hr /> here. For example, we could put the contents into two <CompactCard /> components and that will help split the contents semantically, too - see the last example here.
Or maybe <HrWithText />, <ListEnd />, or even HorizontalRule from Gutenberg.
Essentially, the idea is to reuse as much as possible from the existing components, rather than introducing our own versions. Using the same design language is essential in the long run, and is very valuable when we update that design language and we want the updates to propagate to the entire app.
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 feedback, I have tried to rely as much as possible on existing components, however I did resize and play around to make it more suited to this page.
| stroke="#1D2327" | ||
| strokeWidth="1.5" | ||
| /> | ||
| </svg> |
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.
We could also try using Gutenberg's dashicons - last time I checked there were 3 possible arrow variations we could use here.
| @@ -0,0 +1,104 @@ | |||
| //@import '~@automattic/marketplace/styles/mixins'; | |||
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.
Seems like we could remove this one.
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.
Fixed by 0aa0786
| .marketplace-title { | ||
| font-family: var( --p2-font-inter ); | ||
| letter-spacing: 0.4px; | ||
| line-height: 28px; |
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.
Do we really need to overwrite all those variables? Would be great if we can inherit them and use what's already declared. I think that overlaps a bit with what Jarda mentioned a couple of lines above.
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 was trying to make the design pixel perfect. However I will try to adopt a more inherit based flow 👍
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'd love to discuss this with the design team that worked on it, but my general recommendation would be to use the common interface, components and elements as much as possible, and avoid introducing specific UIs built from scratch, because they end up being hard to maintain in the long run. I've also talked about this in #52081 (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 was trying to make the design pixel perfect.
It shouldn't be the implementation goal to make things pixel perfect. Calypso has a pre-existing design language (actually, several of them 🙃) with conventions for colors, fonts, spacings... often encoded in existing components. New UIs should reuse that as much as possible.
On this page, the domain picker is from Gutenboarding, and the "Your Cart" component is very similar to what we have in Checkout:
What flow is the Yoast page part of? Creating a new site? Or is it an upsell for an existing site? Let's make the page fit in into its surrounding environment.
I think that overlaps a bit with what Jarda mentioned a couple of lines above.
When first seeing the styles, my intuition was: "there's too much CSS here, the UI shouldn't have so many styles". But I didn't know how to express it as something actionable at the time.
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.
Hey @jsnajdr thanks for this feedback, after looking at http://calypso.localhost:3000/devdocs/design I understand what you and @tyxla mean. I will see how I can reuse more existing components to implement this page 😄
This is is the design discussion if you are interested! p9Jlb4-2vT-p2
CC - @olaolusoga @JanaMW27
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 is is the design discussion if you are interested!
Thank you for sharing the context! The discussion seems to all about designing the interactions and navigating the labyrinth of Simple vs Atomic vs different plans combinations and very little about specific icon shapes or colors. That makes me think you have a lot of freedom when implementing these details.
client/my-sites/plugins/marketplace/marketplace-domain-upsell/index.tsx
Outdated
Show resolved
Hide resolved
e956244 to
c316f01
Compare
| }; | ||
| }; | ||
|
|
||
| const theme: MarketplaceThemeType = { |
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.
Little surprised to see a new theme declaration here — don't we have one that just works across Calypso?
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.
Sadly no but I am using css variables wherever possible. Our theme information is mostly embedded in css variables AFAIK.
| <></> | ||
| ) : ( | ||
| <> | ||
| <MarketplaceTitle>Your cart</MarketplaceTitle> |
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.
Note missing __() here and in few other lines in this file.
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 this should be fixed now 👍
d1b626c to
2a355b3
Compare
|
Hi @JanaMW27 @jsnajdr @tyxla most of the feedback you have raised have been addressed. Jana: FYI the free .blog is not yet implemented, it will be implemented as part of the next iteration. Feature vise If it is not an obvious bug the plan is to iterate on other missing parts here on future PRs. Seeing as this PR has grown quite a lot I feel it's best to close the current state of the PR with fixes to code hygiene only. |
…om basket, preload sitename to domain picker
Co-authored-by: Marin Atanasov <[email protected]>
Co-authored-by: Marin Atanasov <[email protected]>
78082e7 to
44ad236
Compare
southp
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 don't see any further major flaws and all the reported cases have been resolved, so let's ship it 🚢
@jsnajdr @tyxla Thanks for your comprehensive review that's informational and educational. If there is any further feedback, please still feel free to let us know and we will be happy to address them as follow-ups :)
tyxla
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 all the hard work here!
Just a heads up that IMHO this looks great from a code perspective 👍
Co-authored-by: Marin Atanasov <[email protected]>
|
This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/5889647 Thank you @jdc91 for including a screenshot in the description! This is really helpful for our translators. |
…pso (#47141) * - Update use-domain-search hook to get title from useTitle hook. - Use React context to provide siteId and update hook to get the correct value always from context. * Update Step by step Launch to use LaunchContext provider. - Remove @wordpress/core-data dependency from editing-toolkit plugin. * Move useOnLaunch hook inside FocusedLaunch. * remove props from Summary component * Domain picker should update domainSearch state using initialDomainSearch prop when showSearchField prop is true. * Fix invalid SVG property * Revert hardcoded value for hasPaidDomain Co-authored-by: Marco Ciampini <[email protected]>
|
Translation for this Pull Request has now been finished. |

Changes proposed in this Pull Request
Testing instructions
Related to #