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

Donation block: Make content editable #43834

Merged
merged 5 commits into from
Jul 2, 2020
Merged

Conversation

mmtr
Copy link
Member

@mmtr mmtr commented Jul 1, 2020

Changes proposed in this Pull Request

Adds the ability to edit the default content included in a donations block.

Jul-02-2020 16-28-00

Testing instructions

  • Generate a new FSE build and sync it to your sandbox: cd apps/full-site-editing; yarn dev --sync.
  • Sandbox the API and the store. More info in PCYsg-lW4-p2 #sandbox-method.
  • Sandbox a test site with a paid plan, and ensure the site has the fse-donations-block blog sticker applied (if not, it can be done from the site's Blog RC).
  • Insert a Donations block in a new post with the block editor.
  • Verify you can toggle all the settings in the right sidebar and the UI updates accordingly.
  • Click through the different tabs and makes sure that content displayed changes accordingly..
  • Try editing the content of the inner blocks.
  • Make sure your changes are preserved.
  • Save the post and reload the editor.
  • Confirm your changes are preserved.

To follow up

Items below are out of scope of this PR and will be implemented in follow-ups.

  • Make currency editable.
  • Make amounts editable (is this required for the MVP)?
  • Save content into the post.
  • Make text/background colors customizable (I guess this is not required for the MVP and can be implemented afterwards).

Part of #42856

@mmtr mmtr requested a review from a team July 1, 2020 15:23
@mmtr mmtr self-assigned this Jul 1, 2020
@matticbot
Copy link
Contributor

@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

@mmtr
Copy link
Member Author

mmtr commented Jul 1, 2020

@gwwar you're welcome to take this over if you want, otherwise I'll continue with it tomorrow.

Most of the issues I'm dealing with are caused by a fragile approach implementing the tab navigation (only one tab should be visible at time, but content of non-visible tabs should be preserved), but I guess there might be blocks that should have solved this before (maybe the slideshow block?).

@@ -72,8 +72,15 @@ const Edit = ( { attributes, setAttributes } ) => {
};

useEffect( () => {
const updateData = () => fetchStatus( 'donation' ).then( mapStatusToState, apiError );
updateData();
const getStatus = async () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

This can be reverted. I changed this to debug easier a failure on my dev, but forgot to leave as it was before.

};

const buttons = {
'one-time': __( 'Donate', 'full-site-editing' ),
Copy link

Choose a reason for hiding this comment

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

ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 79 times:
translate( 'Donate' ) ES Score: 13

className={ classNames( { 'is-active': isTabActive( 'one-time' ) } ) }
onClick={ () => setActiveTab( 'one-time' ) }
>
{ __( 'One-Time', 'full-site-editing' ) }
Copy link

Choose a reason for hiding this comment

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

ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 3 times:
translate( 'one-time' ) ES Score: 7

className={ classNames( { 'is-active': isTabActive( '1 month' ) } ) }
onClick={ () => setActiveTab( '1 month' ) }
>
{ __( 'Monthly', 'full-site-editing' ) }
Copy link

Choose a reason for hiding this comment

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

ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 69 times:
translate( 'Monthly' ) ES Score: 13
See 1 additional suggestions in the PR translation status page

className={ classNames( { 'is-active': isTabActive( '1 year' ) } ) }
onClick={ () => setActiveTab( '1 year' ) }
>
{ __( 'Annually', 'full-site-editing' ) }
Copy link

Choose a reason for hiding this comment

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

ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 18 times:
translate( 'billed annually' ) ES Score: 7
See 1 additional suggestions in the PR translation status page

</div>
</div>
<InspectorControls>
<PanelBody title={ __( 'Settings', 'full-site-editing' ) }>
Copy link

Choose a reason for hiding this comment

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

ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 42 times:
translate( 'Settings', { context: 'NewDash Page Title'} ) ES Score: 8
See 2 additional suggestions in the PR translation status page

@matticbot
Copy link
Contributor

Caution: This PR affects files in the FSE Plugin on WordPress.com
Please ensure your changes work on WordPress.com before merging.

D45779-code has been created so you can easily test it on your sandbox. See this FieldGuide page about developing in the FSE Plugin for more info: PCYsg-ly5-p2

@mmtr
Copy link
Member Author

mmtr commented Jul 2, 2020

Most of the issues I'm dealing with are caused by a fragile approach implementing the tab navigation (only one tab should be visible at time, but content of non-visible tabs should be preserved), but I guess there might be blocks that should have solved this before (maybe the slideshow block?).

Could solve them in aec78f8 by replicating what we did in the premium content block: create a context so the active tab is shared across the nested blocks, allowing them to toggle their visibility.

const { showMonthly, showAnnually, showCustom } = attributes;
return (
<InspectorControls>
<PanelBody title={ __( 'Settings', 'full-site-editing' ) }>
Copy link

Choose a reason for hiding this comment

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

ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 42 times:
translate( 'Settings', { context: 'NewDash Page Title'} ) ES Score: 8
See 2 additional suggestions in the PR translation status page

const { showMonthly, showAnnual, showCustom } = attributes;
return (
<InspectorControls>
<PanelBody title={ __( 'Settings', 'full-site-editing' ) }>
Copy link

Choose a reason for hiding this comment

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

ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 42 times:
translate( 'Settings', { context: 'NewDash Page Title'} ) ES Score: 8
See 2 additional suggestions in the PR translation status page

},
oneTimeButtonText: {
type: 'string',
default: __( 'Donate', 'full-site-editing' ),
Copy link

Choose a reason for hiding this comment

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

ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 79 times:
translate( 'Donate' ) ES Score: 13

const isTabActive = ( tab ) => activeTab === tab;

const tabs = {
'one-time': { title: __( 'One-Time', 'full-site-editing' ) },
Copy link

Choose a reason for hiding this comment

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

ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 3 times:
translate( 'one-time' ) ES Score: 7


const tabs = {
'one-time': { title: __( 'One-Time', 'full-site-editing' ) },
...( showMonthly && { '1 month': { title: __( 'Monthly', 'full-site-editing' ) } } ),
Copy link

Choose a reason for hiding this comment

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

ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 69 times:
translate( 'Monthly' ) ES Score: 13
See 1 additional suggestions in the PR translation status page

const tabs = {
'one-time': { title: __( 'One-Time', 'full-site-editing' ) },
...( showMonthly && { '1 month': { title: __( 'Monthly', 'full-site-editing' ) } } ),
...( showAnnual && { '1 year': { title: __( 'Yearly', 'full-site-editing' ) } } ),
Copy link

Choose a reason for hiding this comment

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

ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 58 times:
translate( 'Yearly' ) ES Score: 15
See 1 additional suggestions in the PR translation status page

@mmtr mmtr changed the title [WIP] Donation block: Make content editable Donation block: Make content editable Jul 2, 2020
@mmtr mmtr added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Jul 2, 2020

const tabs = {
'one-time': { title: __( 'One-Time', 'full-site-editing' ) },
...( monthlyPlanId && { '1 month': { title: __( 'Monthly', 'full-site-editing' ) } } ),
Copy link

Choose a reason for hiding this comment

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

ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 69 times:
translate( 'Monthly' ) ES Score: 13
See 1 additional suggestions in the PR translation status page

const tabs = {
'one-time': { title: __( 'One-Time', 'full-site-editing' ) },
...( monthlyPlanId && { '1 month': { title: __( 'Monthly', 'full-site-editing' ) } } ),
...( annuallyPlanId && { '1 year': { title: __( 'Yearly', 'full-site-editing' ) } } ),
Copy link

Choose a reason for hiding this comment

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

ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 58 times:
translate( 'Yearly' ) ES Score: 15
See 1 additional suggestions in the PR translation status page

@gwwar
Copy link
Contributor

gwwar commented Jul 2, 2020

Make amounts editable (is this required for the MVP)?

Yes, we'll want to store the tier amounts within the block, and have this be editable. This only makes sense for this block since this is technically backed by a pay-what-you-want plan.

@gwwar
Copy link
Contributor

gwwar commented Jul 2, 2020

Nice work @mmtr ! 💖 Looks like we're maybe aiming for a dynamic block format to start with? This does give us a bit more flexibility especially since block updates still aren't the smoothest for folks. I'll go ahead and land this, and will continue work next week.

Screen Shot 2020-07-02 at 3 55 28 PM

@gwwar
Copy link
Contributor

gwwar commented Jul 2, 2020

I also double checked the phpcs failures related to donation locally, and it seems like it's complaining about the asset files which we can ignore.

@gwwar gwwar merged commit 197353c into master Jul 2, 2020
@gwwar gwwar deleted the donation/customize-messages branch July 2, 2020 23:15
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jul 2, 2020
@a8ci18n
Copy link

a8ci18n commented Jul 23, 2020

This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/4124308

Thank you @mmtr for including a screenshot in the description! This is really helpful for our translators.

@a8ci18n
Copy link

a8ci18n commented Jul 27, 2020

Translation for this Pull Request has now been finished.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Donations Donation block
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants