-
Notifications
You must be signed in to change notification settings - Fork 4
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
post migration changes #284
Conversation
…abs/wp-module-ecommerce into post-migration-changes
src/components/OnboardingList.js
Outdated
)} | ||
</Link> | ||
</li> | ||
); | ||
} | ||
|
||
const check_url_match = () => { | ||
const brandName = (NewfoldRuntime?.sdk?.ecommerce?.brand_settings?.name).toLowerCase(); |
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.
NewfoldRuntime?.sdk?.ecommerce
Needs to updated as discussed.
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.
currently in newfoldRuntime, we're not getting ecommerce out of sdk, only once the ecommerce is out side of sdk, we can add this change
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 can't we use NewfoldRuntime?.plugin?.brand
we can also skip toLowerCase()
if we use that variable.
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 can't use plugin, becuase when bluehost changed it's brand like bluehost-us, hostgator_latam, at that time, this value wil not work
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.
As discussed on call we would go split
way and add a TODO
src/configs/OnboardingList.config.js
Outdated
WordPressSdk.settings.get(); | ||
setIsMigrationCompleted(false); | ||
}); | ||
}; | ||
const signUpYoastSEOAcademy = () => { | ||
WordPressSdk.settings.put({ yoast_seo_signup_status: true }); | ||
AnalyticsSdk.track("next_step", "next_step_yoast_academy_clicked", data); | ||
}; | ||
const brandName = | ||
(NewfoldRuntime?.sdk?.ecommerce?.brand_settings?.name).toLowerCase(); |
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.
Avoid using NewfoldRuntime?.sdk
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.
currently in newfoldRuntime, we're not getting ecommerce out of sdk, only once the ecommerce is out side of sdk, we can add this change
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 can't we use NewfoldRuntime?.plugin?.brand
we can also skip toLowerCase()
if we use that variable.
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 can't use plugin, becuase when bluehost changed it's brand like bluehost-us, hostgator_latam, at that time, this value wil not work
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.
As discussed on call we would go split
way and add a TODO
src/configs/OnboardingList.config.js
Outdated
isMigrated: (queries) => queries?.settings?.showMigrationSteps || props.isMigrationCompleted, | ||
}, | ||
|
||
"data-nfdhelpcenterquery": "How do I connect my site to the 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.
Translations not needed?
src/configs/OnboardingList.config.js
Outdated
manage: () => updateSiteServers(props.setWebServersUpdated), | ||
}, | ||
|
||
"data-nfdhelpcenterquery": "How do I update my nameserver to BH?", |
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.
Translations not needed?
src/configs/OnboardingList.config.js
Outdated
isMigrated: (queries) => queries?.settings?.showMigrationSteps, | ||
className: () => "nfd-bg-canvas", | ||
hideCheck: () => true, | ||
showText: () => <a className="nfd-underline" href="https://www.bluehost.com/help" target="_blank">View Guide</a> |
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.
Will this link be hardcoded
to bluehost.com?
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.
@ramyakrishnai plz cross-check this one as well
src/configs/OnboardingList.config.js
Outdated
const signUpYoastSEOAcademy = () => { | ||
WordPressSdk.settings.put({ yoast_seo_signup_status: true }); | ||
AnalyticsSdk.track("next_step", "next_step_yoast_academy_clicked", data); | ||
}; | ||
const brandName = | ||
(NewfoldRuntime?.sdk?.ecommerce?.brand_settings?.name).toLowerCase(); | ||
|
||
const check_url_match = () => { |
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 we avoid redundant code of constant check_url_match
in src/configs/OnboardingList.config.js
and src/components/OnboardingList.js
Define once in src/constants.js
& import as per need.
@ramyakrishnai Please add more details in PR description. |
@@ -21,6 +22,7 @@ function OnboardingCheckListItem({ children, actions, state, ...props }) { | |||
className={classNames( | |||
"nfd-p-[2px]", | |||
"nfd-m-0 nfd-border-b nfd-border-line last:nfd-border-b-0", | |||
state.className, |
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 pass className
as props
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 passing classname as prop which is being destructed as state from props, these values are coming from onboarding.config.js
src/components/OnboardingList.js
Outdated
)} | ||
</Link> | ||
</li> | ||
); | ||
} | ||
|
||
const brandName = (NewfoldRuntime?.sdk?.ecommerce?.brand_settings?.name).toLowerCase(); |
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 use NewfoldRuntime?.plugin?.brand
we can also skip toLowerCase() if we use that variable.
@@ -88,18 +96,18 @@ export function OnboardingScreen({ | |||
)} | |||
> | |||
<div className="nfd-flex nfd-flex-col nfd-justify-start nfd-items-start nfd-gap-4"> | |||
<Title size="2">{title}</Title> | |||
<Title size="2">{isMigrationCompleted ? Text.isMigrated.title : title}</Title> |
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.
Translations need to be added when displaying title
only
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.
translations are added in the variable which is being used here
<div> | ||
{comingSoon ? ( | ||
<Alert | ||
variant="warning" | ||
className="nfd-text-sm nfd-bg-transparent nfd-p-0 " | ||
> | ||
<span className="nfd-text-red-700">{description}</span> | ||
<span className="nfd-text-red-700">{isMigrationCompleted ? Text.isMigrated.description : description}</span> | ||
</Alert> |
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.
Translations need to be added when only description
is displayed
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.
translations are added in the variable which is being used here
src/configs/OnboardingList.config.js
Outdated
WordPressSdk.settings.get(); | ||
setIsMigrationCompleted(false); | ||
}); | ||
}; | ||
const signUpYoastSEOAcademy = () => { | ||
WordPressSdk.settings.put({ yoast_seo_signup_status: true }); | ||
AnalyticsSdk.track("next_step", "next_step_yoast_academy_clicked", data); | ||
}; | ||
const brandName = |
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.
check if brandName
variable could also be added to constants, and imported at necessary places
src/configs/OnboardingList.config.js
Outdated
isMigrated: (queries) => queries?.settings?.showMigrationSteps, | ||
className: () => "nfd-bg-canvas", | ||
hideCheck: () => true, | ||
showText: () => <a className="nfd-underline" href={ NewfoldRuntime.sdk.ecommerce.brand_settings.videGuideLink } target="_blank">View Guide</a> |
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.
videGuideLink
This can be moved out of NewfoldRuntime.sdk.ecommerce.
?
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 create a JS constant
videGuideLink = { 'bluehost': 'https://www.bluehost.com/help', 'hostgator': 'https://www.hostgator.com/help' }
includes/Data/Brands.php
Outdated
@@ -34,7 +34,7 @@ public static function get_config( Container $container ) { | |||
'woocommerce_default_country' => 'AU:NSW', | |||
'woocommerce_currency' => 'AUD', | |||
), | |||
'wondercartBuyNow' => '' | |||
'wondercartBuyNow' => '', |
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 were to remove all Brands.php
changes right?
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.
yeah, the changes were removed already, it's ,
that is changed, I'll try to remove that
Proposed changes
Jira: https://jira.newfold.com/browse/PRESS0-1157
Post Migration steps *****
Given when the migration is successful
When customer is SSO'ed to wp-admin dashboard
Then we will need to display the following options with -> as part of "Next Steps"
"Update your Nameserver to BH"
"Connect the site to your Domain"
And on click of the -> Help Center slider shall open with information for the following questions
How do I update my nameserver to BH?
How do I connect my site to the Domain ?
Need help with these steps ? "View guide" link to https://www.bluehost.com/help
Type of Change
Checklist
Further comments