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

Reporting: Add a parent wrapper component for Payment Activity #8412

Merged
merged 24 commits into from
Mar 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions changelog/add-8389-create-payment-activity-wrapper
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: minor
Type: add

Add a parent wrapper component for Payment Activity widget. This will be visible on the Payments Overview page
30 changes: 30 additions & 0 deletions client/components/payment-activity/index.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/**
* External dependencies
*/
import * as React from 'react';
import { Card, CardBody, CardHeader } from '@wordpress/components';
import { __ } from '@wordpress/i18n';

/**
* Internal dependencies.
*/
import './style.scss';

const PaymentActivity: React.FC = () => {
return (
<Card className="wcpay-payments-activity__card">
<CardHeader
className="wcpay-payments-activity__card__header"
isBorderless={ true }
>
Comment on lines +15 to +19
Copy link
Member

Choose a reason for hiding this comment

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

💭 There may be an opportunity to reduce the amount of custom css applied here.

I attempted the following and see no major difference with the rendered component (yet, I know there may be more to come, but we can let that be implemented as-needed).

Suggested change
<Card className="wcpay-payments-activity__card">
<CardHeader
className="wcpay-payments-activity__card__header"
isBorderless={ true }
>
<Card>
<CardHeader>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, @Jinksi. I have added these classes since:

  1. I found that the Card by default does not have any padding and the inner individual constituents have their own padding. I thought it is better to add the padding to the whole wrapper as per our design, for uniformity. This will also help have same padding for upcoming components.

  2. The CSS on header card removes its own padding and there is an isBorderless prop set to true to remove bottom border on the header.

I hope this makes sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree with @Jinksi here – not so much "let's avoid css classes", more that we should avoid overriding / customising styles.

If the components aren't doing what we want (i.e. differ from design in subtle or major ways), we should:

  • adapt the design to match the components - i.e. we can implement the intended design but it looks a little different (though likely more consistent with rest of app)
  • or
  • adapt the components so they have the flexibility we need - i.e. this design proposes changes or additions to components, and we need to implement in component

Copy link
Contributor Author

@nagpai nagpai Mar 28, 2024

Choose a reason for hiding this comment

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

Thanks for sharing some very helpful guidance. I removed the custom css, and found the only difference it made was adding some extra 8 px padding. I also now see that has been removed in the new simplified design update, making it consistent with the headings of other widgets.

The latest design also has a bottom border ( default view ) for the card header.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since I had already merged this PR before @haszari 's comment, i have created a fresh PR with the tweak

#8507

{ __( 'Your payment activity', 'woocommerce-payments' ) }
{ /* Filters go here */ }
</CardHeader>
<CardBody className="wcpay-payments-activity__card__body">
<>{ /* Sub components go here */ }</>
</CardBody>
</Card>
);
};

export default PaymentActivity;
15 changes: 15 additions & 0 deletions client/components/payment-activity/style.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
.wcpay-payments-activity {
&__card {
padding: 24px 24px 16px 24px;

&__header {
padding: 0 !important;
margin-bottom: 32px;
}

&__body {
text-align: center;
color: $studio-gray-30;
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`PaymentActivity component should render 1`] = `
<div>
<div
class="components-surface components-card wcpay-payments-activity__card css-nsno0f-View-Surface-getBorders-primary-Card-rounded em57xhy0"
data-wp-c16t="true"
data-wp-component="Card"
>
<div
class="css-mgwsf4-View-Content em57xhy0"
>
<div
class="components-flex components-card__header components-card-header wcpay-payments-activity__card__header css-sz55ca-View-Flex-sx-Base-sx-Items-ItemsRow-Header-borderRadius-borderColor-medium-borderless em57xhy0"
data-wp-c16t="true"
data-wp-component="CardHeader"
>
Your payment activity
</div>
<div
class="components-card__body components-card-body wcpay-payments-activity__card__body css-1nwhnu3-View-Body-borderRadius-medium em57xhy0"
data-wp-c16t="true"
data-wp-component="CardBody"
/>
</div>
<div
aria-hidden="true"
class="components-elevation css-91yjwm-View-Elevation-sx-Base-elevationClassName em57xhy0"
data-wp-c16t="true"
data-wp-component="Elevation"
/>
<div
aria-hidden="true"
class="components-elevation css-91yjwm-View-Elevation-sx-Base-elevationClassName em57xhy0"
data-wp-c16t="true"
data-wp-component="Elevation"
/>
</div>
</div>
`;
18 changes: 18 additions & 0 deletions client/components/payment-activity/test/index.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/**
* External dependencies
*/
import React from 'react';
import { render } from '@testing-library/react';

/**
* Internal dependencies
*/
import PaymentActivity from '..';

describe( 'PaymentActivity component', () => {
it( 'should render', () => {
const { container } = render( <PaymentActivity /> );

expect( container ).toMatchSnapshot();
} );
} );
49 changes: 31 additions & 18 deletions client/overview/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,25 +11,26 @@ import { __ } from '@wordpress/i18n';
/**
* Internal dependencies.
*/
import Page from 'components/page';
import { TestModeNotice } from 'components/test-mode-notice';
import AccountStatus from 'components/account-status';
import Welcome from 'components/welcome';
import AccountBalances from 'components/account-balances';
import DepositsOverview from 'components/deposits-overview';
import AccountStatus from 'components/account-status';
import ActiveLoanSummary from 'components/active-loan-summary';
import ConnectionSuccessNotice from './connection-sucess-notice';
import DepositsOverview from 'components/deposits-overview';
import ErrorBoundary from 'components/error-boundary';
import TaskList from './task-list';
import { getTasks, taskSort } from './task-list/tasks';
import FRTDiscoverabilityBanner from 'components/fraud-risk-tools-banner';
import JetpackIdcNotice from 'components/jetpack-idc-notice';
import Page from 'components/page';
import PaymentActivity from 'components/payment-activity';
import Welcome from 'components/welcome';
import { TestModeNotice } from 'components/test-mode-notice';
import InboxNotifications from './inbox-notifications';
import ConnectionSuccessNotice from './connection-sucess-notice';
import ProgressiveOnboardingEligibilityModal from './modal/progressive-onboarding-eligibility';
import JetpackIdcNotice from 'components/jetpack-idc-notice';
import FRTDiscoverabilityBanner from 'components/fraud-risk-tools-banner';
import { useDisputes, useGetSettings, useSettings } from 'wcpay/data';
import SetupLivePaymentsModal from './modal/setup-live-payments';
import strings from './strings';
import TaskList from './task-list';
import { getTasks, taskSort } from './task-list/tasks';
import { useDisputes, useGetSettings, useSettings } from 'data';
import './style.scss';
import SetupLivePaymentsModal from './modal/setup-live-payments';

const OverviewPageError = () => {
const queryParams = getQuery();
Expand All @@ -55,10 +56,13 @@ const OverviewPageError = () => {
const OverviewPage = () => {
const {
accountStatus,
accountStatus: { progressiveOnboarding },
accountLoans: { has_active_loan: hasActiveLoan },
enabledPaymentMethods,
featureFlags: { isPaymentOverviewWidgetEnabled },
overviewTasksVisibility,
showUpdateDetailsTask,
wpcomReconnectUrl,
enabledPaymentMethods,
} = wcpaySettings;

const isDevMode = wcpaySettings.devMode;
Expand Down Expand Up @@ -95,8 +99,8 @@ const OverviewPage = () => {
queryParams[ 'wcpay-server-link-error' ] === '1';
const showProgressiveOnboardingEligibilityModal =
showConnectionSuccess &&
accountStatus.progressiveOnboarding.isEnabled &&
! accountStatus.progressiveOnboarding.isComplete;
progressiveOnboarding.isEnabled &&
! progressiveOnboarding.isComplete;
const showTaskList =
! accountRejected && ! accountUnderReview && tasks.length > 0;

Expand Down Expand Up @@ -191,18 +195,27 @@ const OverviewPage = () => {
<AccountBalances />
</Card>
) }

{
/* Show Payment Activity widget only when feature flag is set. To be removed before go live */
isPaymentOverviewWidgetEnabled && (
<Card>
<ErrorBoundary>
<PaymentActivity />
</ErrorBoundary>
</Card>
)
}
<DepositsOverview />
</>
</ErrorBoundary>
) }
<ErrorBoundary>
<AccountStatus
accountStatus={ wcpaySettings.accountStatus }
accountStatus={ accountStatus }
accountFees={ activeAccountFees }
/>
</ErrorBoundary>
{ wcpaySettings.accountLoans.has_active_loan && (
{ hasActiveLoan && (
<ErrorBoundary>
<ActiveLoanSummary />
</ErrorBoundary>
Expand Down
Loading