-
Notifications
You must be signed in to change notification settings - Fork 25
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
feature / bookings ctb added in pages and posts #1183
Conversation
@manikantakailasa Thanks for your PR.
If the items above are resolved by tomorrow, we can include your PR in the July 24 release. |
@wpalani |
@@ -0,0 +1,35 @@ | |||
import { Button, Card, Title } from '@newfold/ui-component-library'; | |||
import { ReactComponent as TransformLogo } from '../../../../assets/svg/transformStore.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.
Please use the Assets
path alias to simplify the import path.
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.
tried it but tests are failing as it is not able to find the path.
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 inspected the Vector and it looks like the Vector was converted (By the designer) from an image, which increased the size of the asset from 2kb to 45kb with no added value in our use case.
I think it's best we convert this back from svg
to png
. We also need to properly name the asset to tie back the component name.
I created a branch from your PR and made some changes to simplify the import and component naming. Check this commit: b914f43
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 initially approved this PR. But took a second look at the Jira story and it this PR doesn't seem to properly address it.
import { Button, Card, Title } from '@newfold/ui-component-library'; | ||
import { ReactComponent as TransformLogo } from '../../../../assets/svg/transformStore.svg'; | ||
|
||
const TransformStore = () => { |
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.
Change component name to EcommAddonCTB
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.
updated
@@ -0,0 +1,35 @@ | |||
import { Button, Card, Title } from '@newfold/ui-component-library'; | |||
import { ReactComponent as TransformLogo } from '../../../../assets/svg/transformStore.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.
I inspected the Vector and it looks like the Vector was converted (By the designer) from an image, which increased the size of the asset from 2kb to 45kb with no added value in our use case.
I think it's best we convert this back from svg
to png
. We also need to properly name the asset to tie back the component name.
I created a branch from your PR and made some changes to simplify the import and component naming. Check this commit: b914f43
{ window.NewfoldRuntime.isYithBookingActive && | ||
window.NewfoldRuntime.isWoocommerceActive && ( | ||
<BookingAndAppointments /> | ||
) } | ||
window.NewfoldRuntime.isWoocommerceActive ? ( | ||
<BookingAndAppointments /> | ||
) : ( | ||
<TransformStore /> | ||
) } |
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 Jira story requirement states:
Given customers do not have an ecommerce entitlement
When they view Pages and Posts (launchpad) in wp-admin
Then we will have CTB for ecom addon in the place of bookings card instead of removing the card altogether.
Meaning: this card (eComm addon CTB) is intended to be shown to users who do not have the e-commerce addon in their plan (ecommerce entitlement). In this condition, we are not considering whether they have the addon or not.
The condition should be:
IF: WooCommerce is active
AND: The user doesn't have the e-commerce addon
THEN: Show the `EcommAddonCTB` component
ELSE IF...
IF: WooCommerce is active
AND: Yith Booking is active
THEN: Show `BookingAndAppointments` component
ELSE:
Show nothing
END
You can check for the e-commerce entitlement using the Newfold runtime:
import { NewfoldRuntime } from '@newfold-labs/wp-module-runtime';
NewfoldRuntime.hasCapability( 'isEcommerce' ) // Bool
Or you can:
window.NewfoldRuntime.capabilities.isEcommerce // Bool | Undefined
Be careful with the second one because it can return undefined
, especially in local environments or sites with connection issue.
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.
updated fix , but ecommerce entitlement we check with hasYithExtended.
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's the reason for not using isEcommerce
?
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.
isEcommerce refers to whether or not WooCommerce plugin is active, has nothing to do with the commerce addon.
@@ -1,68 +1,68 @@ | |||
describe( 'Pages & Posts', function () { | |||
describe('Pages & Posts', function () { |
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.
- Please update the tests in this file to reflect the story requirements accurately.
- Please revert the spaces of the lint.
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
When they view Pages and Posts (launchpad) in wp-admin
Then we will have CTB for bookings instead of removing the card altogether if they dont have bookings installed.
pages and posts.mov.zip
story: https://jira.newfold.com/browse/PRESS0-1323