-
Notifications
You must be signed in to change notification settings - Fork 8
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
Api integration preview home page #380
Api integration preview home page #380
Conversation
…bs/wp-module-onboarding into enhance/aio-design-previews # Conflicts: # src/OnboardingSPA/styles/app.scss
…dule-onboarding into enhance/aio-preview
…newfold-labs/wp-module-onboarding into api-integration-preview-home-page
…/wp-module-onboarding into enhance/aio-preview
…bs/wp-module-onboarding into api-integration-preview-home-page
…bs/wp-module-onboarding into api-integration-preview-home-page # Conflicts: # src/OnboardingSPA/styles/_branding.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.
- Let's use
Options::get_option_name()
whenever possible - Let's Format the code correctly as rn the PHP Lint check has issues.
$target_audience = get_option('nfd-ai-site-gen-targetaudience'); | ||
$content_style = get_option('nfd-ai-site-gen-contenttones'); |
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 am pretty skeptical about this though.
const { headers, headerActiveView, isHeaderEnabled, currentStep } = | ||
useSelect( ( select ) => { | ||
return { | ||
currentStep: select( nfdOnboardingStore ).getCurrentStep(), |
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.
Maybe we can keep it in the same order i.e. var names and functions.
src/OnboardingSPA/components/LivePreview/SiteGenPreviewSelectableCard/index.js
Show resolved
Hide resolved
…bs/wp-module-onboarding into api-integration-preview-home-page
…bs/wp-module-onboarding into api-integration-preview-home-page
…bs/wp-module-onboarding into api-integration-preview-home-page
…bs/wp-module-onboarding into api-integration-preview-home-page
if ( ! $target_audience || ! $content_style ) { | ||
return new \WP_Error( | ||
'nfd_onboarding_error', | ||
__( 'Required data is missing.', 'wp-module-onboarding' ), | ||
array( | ||
'status' => 400, | ||
) | ||
); | ||
} |
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.
What if we add a code to send Error from our Service and catch it here, cause I believe here what if it gives an error from Press 3 API, that won't be null but still have an error and going forward will break things.
$target_audience = SiteGenService::instantiate_site_meta( $site_info, 'targetaudience', true ); | ||
$content_style = SiteGenService::instantiate_site_meta( $site_info, 'contenttones', true ); | ||
|
||
if ( ! $target_audience || ! $content_style ) { | ||
return new \WP_Error( | ||
'nfd_onboarding_error', | ||
__( 'Required data is missing.', 'wp-module-onboarding' ), | ||
array( | ||
'status' => 400, | ||
) | ||
); | ||
} |
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.
Same as above
const regenerate = () => { | ||
// alert( 'regenerate' ); | ||
}; |
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 this supposed to be like this?
…bs/wp-module-onboarding into api-integration-preview-home-page
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.
Looks good to me!
home page preview: