feat(donate-block): hide buttons if only one frequency is available #1853

adekbadek · 2024-08-23T12:29:02Z

All Submissions:

Have you followed the Newspack Contributing guideline?
Does your code follow the WordPress' coding standards and VIP Go coding standards?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes proposed in this Pull Request:

If the Donate block is set to render just one frequency, the frequency buttons section should not be displayed.
Instead, a frequency label is added, if applicable.

Before	After

How to test the changes in this Pull Request:

On trunk, insert Donate blocks (see below for a copy-paste)
Observe the frequency choosing UI is rendered, with a single button
Switch to this branch, reload editor & frontend – observe the selection UI is not rendered and frequency labels added instead, where applicable

Blocks code for testing

<!-- wp:newspack-blocks/donate {"className":"is-style-default","manual":true,"amounts":{"once":[9,20,90,20],"month":[7,15,30,15],"year":[84,180,360,180]},"disabledFrequencies":{"once":false,"month":true,"year":true},"defaultFrequency":"once","minimumDonation":5} /-->
<!-- wp:newspack-blocks/donate {"className":"is-style-default","manual":true,"amounts":{"once":[9,20,90,20],"month":[7,15,30,15],"year":[84,180,360,180]},"disabledFrequencies":{"once":true,"month":false,"year":true},"minimumDonation":5} /-->
<!-- wp:newspack-blocks/donate {"className":"is-style-default","manual":true,"amounts":{"once":[9,20,90,20],"month":[7,15,30,15],"year":[84,180,360,180]},"disabledFrequencies":{"once":true,"month":true,"year":false},"defaultFrequency":"year","minimumDonation":5} /-->
<!-- wp:newspack-blocks/donate {"className":"is-style-default","manual":true,"tiered":false,"amounts":{"once":[9,20,90,20],"month":[7,15,30,15],"year":[84,180,360,180]},"disabledFrequencies":{"once":false,"month":true,"year":true},"defaultFrequency":"once","minimumDonation":5} /-->
<!-- wp:newspack-blocks/donate {"className":"is-style-default","manual":true,"tiered":false,"amounts":{"once":[9,20,90,20],"month":[7,15,30,15],"year":[84,180,360,180]},"disabledFrequencies":{"once":true,"month":false,"year":true},"minimumDonation":5} /-->
<!-- wp:newspack-blocks/donate {"className":"is-style-default","manual":true,"tiered":false,"amounts":{"once":[9,20,90,20],"month":[7,15,30,15],"year":[84,180,360,180]},"disabledFrequencies":{"once":true,"month":true,"year":false},"defaultFrequency":"year","minimumDonation":5} /-->

Other information:

Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes, as applicable?
Have you successfully ran tests with your changes locally?

To see the specific tasks where the Asana app for GitHub is being used, see below:
- https://app.asana.com/0/0/1208106329606187

kevinzweerink · 2024-08-23T13:31:01Z

Hmm, I'm a bit hesitant about this — it seems too easy for a publisher to accidentally set up something where their description doesn't match the behavior, i.e. a donation block is set to weekly, and the publisher mistakenly adds copy above it saying "Make a one-time donation." Even if the user notices that it's actually recurring in the checkout flow, they might feel mislead and abandon the donation.

What do you think about this idea: in cases where only one frequency is selected, that title is expanded to "Make a {frequency} donation."? That way it's more clear, but removes the risk of mislabeling a recurring product.

adekbadek · 2024-08-26T09:40:20Z

@kevinzweerink – the original request from a publisher was to hide the label. WDYT about making this an option? Then a publisher can hide the label (as an explicit step) and add their own using e.g. a paragraph block above the Donate block?

kevinzweerink · 2024-09-04T13:58:31Z

Sure, we could do that. I don't love this direction in general because I feel like it introduces more hidden functionality to the donation block, which already features a lot of configuration options that you have to know to look for. At this stage I'm not sure it makes sense to revisit the general approach though.

If we want to go ahead and implement this, here's a mockup of where I think the control should live — I would make it a toggle that appears under the tier options when only one tier is selected; this way it's clear which label will be removed.

ronchambers · 2024-09-10T18:37:33Z

Hi Adam and Kevin. I was invited by Claudiu to get up to speed on a number of Newspack products and dev environment in order to help out as needed. He recommended I give a review to a number of PRs, including this one.

I'll provide my review here if it helps in anyway 😃

Overall I think this is a tough one. I understand publishers wanting to remove an unneeded label when there is only one "tab", but I also understand that it could cause a publisher to accidentally sell a frequency that the user doesn't see.

How about the following:

I noticed there are other configurations of the Donate Block that already show a "redundant" frequency in both the tab and in the content area.

Please see these two screenshots:

So maybe this same "redundancy" can be applied to the following layouts. This way, if there is only one tab/label and it is auto-hidden, the user can still see the frequency in the content box.

Please see these 3 mockups:

In the end, my thoughts would be to do:

Keep the code in this PR that automatically hides the single tab when there is only one label/frequency. I think this is good and "makes sense" to not have an unneeded single tab. Also I would assume (but can't confirm) that from an Accessibility point of view it's probably best to remove the single tab or at minimum remove the single tab's click-action/link so accessible browsers are not confused by a single tab that doesn't change anything when clicked.
Add the redundant frequency into the block content area for the 3 "mockup" layouts above. This will make all 5 layouts more similar in design. Be sure to keep this new text on all screen sizes, don't hide on mobile.
With this approach we should be able to avoid any new toggles since I agree there is quite a bit going on in the configurations.

I hope this might help :-)

Thanks!

adekbadek · 2024-09-19T11:26:25Z

Thanks @ronchambers ! I like the idea. WDYT @kevinzweerink ?

adekbadek · 2024-10-07T10:35:42Z

Thanks @ronchambers! I've applied your suggestions.

ronchambers · 2024-10-10T03:11:49Z

@adekbadek - Thanks for these changes. If you ever need me to QA/review sooner, feel free to let me know a deadline or ping me on slack too :-)

Overall I really like the new "per month" and "per year" labels on all the different styles. And I really like the new sentence "Donation amount per month" and "Donation amount per year". I also noticed that a single-tab will disappear when there is only one tab. Awesome!

I have two pieces of feedback:

I noticed a small bug for the "default" style with the attached configuration. When clicking "other" the label should be hidden/removed like in the other styles.

I think we should put a label for "one-time or once" like you just did for "per month" / "per year". For the sentence I would recommend "Donation amount (one-time)" with parenthesis. I'll attached the idea. (I prefer the word "one-time" instead of "once" if possible...) If it's easier to put the "()" parenthesis around the label too, that seems fine with me.

adekbadek · 2024-10-11T09:16:26Z

I noticed a small bug for the "default" style with the attached configuration. […]

Good catch! Fixed in bbed5ad

I think we should put a label for "one-time or once" like you just did for "per month" / "per year".

I'm not a fan of how this looks. I think it's fine to assume that a donation is a one-time donation.

ronchambers · 2024-10-14T06:15:48Z

Nice, I retested and bug is fixed. As for adding "one-time" or "once", personally I think something is needed since users and publishers "could" get confused, but I also think what you said is reasonable in that most people would probably assume a donation is "one-time" unless stated otherwise. So cool with me either way :-)

ronchambers

I tested the functionality and it works as described. I did not do a code review - I just tested via the wp-admin / blocks editor.

dkoo · 2024-10-17T16:05:40Z

@adekbadek is this ready to merge?

adekbadek · 2024-10-23T09:12:44Z

Sorry, forgot about this one. It would use a code review on top of QA. I've asked @ronchambers on Slack.

miguelpeixe

Code looks good, except for the linting questions I left inline.

The tiered block in the editor has a small misalignment on the "per month" text. On the page it looks good.

Editor	Page

miguelpeixe · 2024-11-13T20:13:19Z

webpack.config.js

+	const pathToBlock = [__dirname, 'src', 'blocks', block]
+	let viewScriptPath = path.join( ...pathToBlock, 'view.js' );
+	let fileExists = fs.existsSync( viewScriptPath )
+	if (!fileExists) {
+		// Try TS.
+		viewScriptPath = path.join( ...pathToBlock, 'view.ts' );
+		fileExists = fs.existsSync( viewScriptPath )
+	}
+	if ( fileExists ) {


There's some inconsistent spacing and missing ; here.

Fixed in f40d831

miguelpeixe · 2024-11-13T20:16:38Z

src/blocks/donate/view.ts

@@ -0,0 +1,53 @@
+/**


Did we change our linting rules? This file does not follow our usual practices.

There's something odd indeed, stemming from #1774. When I format the code of a file untouched by the changes here, with the npm script, it does make edits. For example:

$ ./node_modules/.bin/newspack-scripts wp-scripts lint-js ./src/blocks/author-list/edit.js -> no errors, linting is ok $ ./node_modules/.bin/newspack-scripts wp-scripts format ./src/blocks/author-list/edit.js -> edits are made to the file $ ./node_modules/.bin/newspack-scripts wp-scripts lint-js ./src/blocks/author-list/edit.js -> still no linting errors, despite the fact that the file is edited and all formatting changed

But that's not something introduced by this PR, so it's a subject for another discussion.

adekbadek · 2024-11-14T09:45:17Z

The tiered block in the editor has a small misalignment on the "per month" text. On the page it looks good.

I can't reproduce:

Can you make sure that the CSS is rebuild and this CSS applied (via the inspector)?

miguelpeixe

Thanks, @adekbadek! Not sure what happened there, it's rendering fine now.

github-actions · 2024-11-15T08:23:32Z

Hey @, good job getting this PR merged! 🎉

Now, the needs-changelog label has been added to it.

Please check if this PR needs to be included in the "Upcoming Changes" and "Release Notes" doc. If it doesn't, simply remove the label.

If it does, please add an entry to our shared document, with screenshots and testing instructions if applicable, then remove the label.

Thank you! ❤️

# [4.4.0-alpha.1](v4.3.5...v4.4.0-alpha.1) (2024-11-15) ### Features * **donate-block:** hide buttons if only one frequency is available ([#1853](#1853)) ([043591e](043591e)) * **homepage-articles:** rename block and reorganise settings ([#1943](#1943)) ([1af11a4](1af11a4))

matticbot · 2024-11-15T20:26:33Z

🎉 This PR is included in version 4.4.0-alpha.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

# [4.4.0](v4.3.7...v4.4.0) (2024-11-25) ### Bug Fixes * revert "feat(homepage-articles): rename block and reorganise settings ([#1943](#1943))" ([#1971](#1971)) ([fb161b9](fb161b9)) ### Features * add Bluesky support to the Author Profile, List blocks ([#1969](#1969)) ([f1c29ab](f1c29ab)) * **donate-block:** hide buttons if only one frequency is available ([#1853](#1853)) ([043591e](043591e)) * **homepage-articles:** rename block and reorganise settings ([#1943](#1943)) ([1af11a4](1af11a4))

matticbot · 2024-11-25T19:19:02Z

🎉 This PR is included in version 4.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

feat(donate-block): hide buttons if only one frequency is available

2db21cb

adekbadek added the [Status] Needs Review label Aug 23, 2024

adekbadek requested a review from a team as a code owner August 23, 2024 12:29

adekbadek added 2 commits August 26, 2024 11:11

refactor: remove duplication

28433a6

fix: tiers-based PHP

b69d966

Merge branch 'trunk' into feat/donate-single-frequency

5d1dc90

adekbadek and others added 3 commits October 7, 2024 11:18

Merge branch 'trunk' into feat/donate-single-frequency

7885161

feat: add frequency labels for disambiguation

a7f532d

feat: move the freq. label out of the button

7508d33

fix: hide label if custom amount is chosen

bbed5ad

Merge branch 'trunk' into feat/donate-single-frequency

1e89afe

ronchambers approved these changes Oct 15, 2024

View reviewed changes

github-actions bot added [Status] Approved and removed [Status] Needs Review labels Oct 15, 2024

Merge branch 'trunk' into feat/donate-single-frequency

bf35f01

miguelpeixe requested changes Nov 13, 2024

View reviewed changes

style: webpack file formatting

f40d831

adekbadek requested a review from miguelpeixe November 14, 2024 09:45

miguelpeixe approved these changes Nov 14, 2024

View reviewed changes

adekbadek merged commit 043591e into trunk Nov 15, 2024
10 checks passed

adekbadek deleted the feat/donate-single-frequency branch November 15, 2024 08:23

github-actions bot added the needs-changelog label Nov 15, 2024

adekbadek removed the needs-changelog label Nov 15, 2024

matticbot added the released on @alpha label Nov 15, 2024

matticbot added the released label Nov 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(donate-block): hide buttons if only one frequency is available #1853

feat(donate-block): hide buttons if only one frequency is available #1853

adekbadek commented Aug 23, 2024 •

edited

Loading

kevinzweerink commented Aug 23, 2024

adekbadek commented Aug 26, 2024 •

edited

Loading

kevinzweerink commented Sep 4, 2024

ronchambers commented Sep 10, 2024 •

edited

Loading

adekbadek commented Sep 19, 2024

adekbadek commented Oct 7, 2024

ronchambers commented Oct 10, 2024

adekbadek commented Oct 11, 2024

ronchambers commented Oct 14, 2024

ronchambers left a comment

dkoo commented Oct 17, 2024

adekbadek commented Oct 23, 2024

miguelpeixe left a comment

miguelpeixe Nov 13, 2024

adekbadek Nov 14, 2024

miguelpeixe Nov 13, 2024

adekbadek Nov 14, 2024

adekbadek commented Nov 14, 2024

miguelpeixe left a comment

github-actions bot commented Nov 15, 2024

matticbot commented Nov 15, 2024

matticbot commented Nov 25, 2024

feat(donate-block): hide buttons if only one frequency is available #1853

feat(donate-block): hide buttons if only one frequency is available #1853

Conversation

adekbadek commented Aug 23, 2024 • edited Loading

All Submissions:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Other information:

kevinzweerink commented Aug 23, 2024

adekbadek commented Aug 26, 2024 • edited Loading

kevinzweerink commented Sep 4, 2024

ronchambers commented Sep 10, 2024 • edited Loading

adekbadek commented Sep 19, 2024

adekbadek commented Oct 7, 2024

ronchambers commented Oct 10, 2024

adekbadek commented Oct 11, 2024

ronchambers commented Oct 14, 2024

ronchambers left a comment

Choose a reason for hiding this comment

dkoo commented Oct 17, 2024

adekbadek commented Oct 23, 2024

miguelpeixe left a comment

Choose a reason for hiding this comment

miguelpeixe Nov 13, 2024

Choose a reason for hiding this comment

adekbadek Nov 14, 2024

Choose a reason for hiding this comment

miguelpeixe Nov 13, 2024

Choose a reason for hiding this comment

adekbadek Nov 14, 2024

Choose a reason for hiding this comment

adekbadek commented Nov 14, 2024

miguelpeixe left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 15, 2024

matticbot commented Nov 15, 2024

matticbot commented Nov 25, 2024

adekbadek commented Aug 23, 2024 •

edited

Loading

adekbadek commented Aug 26, 2024 •

edited

Loading

ronchambers commented Sep 10, 2024 •

edited

Loading