Tidy up after recent Focused Launch checkout changes #50182
Conversation
| @@ -54,6 +55,7 @@ registerPlugin( 'a8c-editor-site-launch', { | |||
| redirectTo: redirectToWpcomPath, | |||
| openCheckout, | |||
| getCurrentLaunchFlowUrl, | |||
| isInIframe: inIframe(), | |||
There was a problem hiding this comment.
Changes in 48452 introduced this error (the isInIframe prop is required by LaunchContext but was not provided here), but somehow neither building the site locally, nor the CI checks flagged it.
We should look into having a full automated TypeScript check — pinging @scinos for some guidance (and in case this is related to #49984)
There was a problem hiding this comment.
We've set isInIframe to false as default in the commit where we introduced it and the app mounted in attach-launch-sidebar.tsx is not using it so it's not really a bug.
I'm not against adding it to have the context consistent across both apps but maybe we should remove the default to reduce the maintenance effort?
Good to read on the context subject: https://kentcdodds.com/blog/how-to-use-react-context-effectively
Tl;dr:
- we can do this to avoid TS errors:
const LaunchContext = React.createContext< LaunchContext | undefined >( undefined ); - in unit tests, unlike stated in React docs, we should wrap the components with the necessary context and not rely on this default value
There was a problem hiding this comment.
I see why this wasn't a real bug, but it caused a TypeScript error that was never flagged (the only way I came across it was by opening that file in my IDE).
I agree that one way of fixing this is to make the isInIframe prop optional — I will go ahead and apply this change in this PR
There was a problem hiding this comment.
I agree that one way of fixing this is to make the
isInIframeprop optional — I will go ahead and apply this change in this PR
It's probably not a good idea to do that since we have it's required in Focused Launch 😬
Let's continue to keep the context using the same interface for both apps and set a value to this property even if it's not used at the moment in this variation.
There was a problem hiding this comment.
Ok then, I guess we can keep the code in this PR as is.
maybe we should remove the default value of the
LaunchContext
This could be an option.
The more pressing issue, for me, is to understand how we can get TypeScript errors like this flagged in our automated build / CI processes
matticbot
added
the
[Status] Needs Review
ciampo
added
[Type] Janitorial
Focused Launch
|
Caution: This PR affects files in the Editing Toolkit Plugin on WordPress.com D57191-code has been created so you can easily test it on your sandbox. See this FieldGuide page about developing the Editing Toolkit Plugin for more info: PCYsg-ly5-p2 |
ciampo
force-pushed
the
feat/focused-launch-comments
branch
from
3b45cd1 to
3f6df32
Compare
ciampo
added
the
Editing Toolkit
|
This PR does not affect the size of JS and CSS bundles shipped to the user's browser. Generated by performance advisor bot at iscalypsofastyet.com. |
ciampo
changed the title
Co-authored-by: Răzvan Papadopol <[email protected]>
Co-authored-by: Răzvan Papadopol <[email protected]>
Co-authored-by: Răzvan Papadopol <[email protected]>
|
@razvanpapadopol is this PR good to go, or do you see any other changes required? |
| * @param siteSlug The slug (id) of the current site. | ||
| * @param isEcommerce True if the eCommerce plan is going to be in the checkout. | ||
| * @param onSuccessCallback Called when the checkout opens as a modal and is completed successfully | ||
| */ | ||
| export const openCheckout = ( | ||
| siteSlug: string = window?.currentSiteId?.toString() || '', |
There was a problem hiding this comment.
Just verified. window.currentSiteId doesn't exist. window._currentSiteId does. Then we can also fix the typings.
There was a problem hiding this comment.
Cleaned up that variable in 4d9ec5f
There was a problem hiding this comment.
@yansern can you have another look?
Looks good! :)
|
@yansern can you have another look? |
matticbot
removed
the
[Status] Needs Review
Changes proposed in this Pull Request
Following work from #48452:
isInIframeprop for the Step by Step launch contextwindow.currentSiteIdtowindow._currentSiteId(and fixed related TypeScript type)Testing instructions
Most changes are just comments.
apps/editing-toolkit/editing-toolkit-plugin/editor-site-launch/src/attach-launch-sidebar.tsxand verify that the TypeScript error is goneRelated to #48452