Skip to content
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

Merged
merged 10 commits into from
Aug 7, 2024

Conversation

manikantakailasa
Copy link
Contributor

@manikantakailasa manikantakailasa commented Jul 15, 2024

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

@wpalani wpalani added this to the July 24 Release milestone Jul 15, 2024
@wpalani wpalani self-requested a review July 17, 2024 21:06
@wpalani
Copy link
Member

wpalani commented Jul 17, 2024

@manikantakailasa Thanks for your PR.

  • Since your PR includes UI changes, can you include a screenshot/video of the change?
  • Fix failing tests. I believe there's an issue with the SVG asset.
  • Can you add a cypress test? I think we asked for that in your last PR that was closed: Press0 1323 | bookings ctb added in pages and posts  #1119
  • Fix lint issues.

If the items above are resolved by tomorrow, we can include your PR in the July 24 release.

@manikantakailasa
Copy link
Contributor Author

@manikantakailasa Thanks for your PR.

  • Since your PR includes UI changes, can you include a screenshot/video of the change?
  • Fix failing tests. I believe there's an issue with the SVG asset.
  • Can you add a cypress test? I think we asked for that in your last PR that was closed: Press0 1323 | bookings ctb added in pages and posts  #1119
  • Fix lint issues.

If the items above are resolved by tomorrow, we can include your PR in the July 24 release.

@wpalani
made changes as per suggestions and attached video in notes.

@@ -0,0 +1,35 @@
import { Button, Card, Title } from '@newfold/ui-component-library';
import { ReactComponent as TransformLogo } from '../../../../assets/svg/transformStore.svg';
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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

@wpalani wpalani changed the base branch from develop to update/PRESS0-1323 July 24, 2024 21:35
@wpalani wpalani self-requested a review July 26, 2024 22:53
Copy link
Member

@wpalani wpalani left a 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 = () => {
Copy link
Member

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

Copy link
Contributor Author

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';
Copy link
Member

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

Comment on lines 81 to 86
{ window.NewfoldRuntime.isYithBookingActive &&
window.NewfoldRuntime.isWoocommerceActive && (
<BookingAndAppointments />
) }
window.NewfoldRuntime.isWoocommerceActive ? (
<BookingAndAppointments />
) : (
<TransformStore />
) }
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Contributor

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 () {
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@manikantakailasa manikantakailasa changed the base branch from update/PRESS0-1323 to develop August 2, 2024 11:02
@wpalani wpalani merged commit 4923d40 into bluehost:develop Aug 7, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants