Only show five inbox messages and add "Show more" button #35003

joelclimbsthings · 2022-10-07T22:33:56Z

All Submissions:

Have you followed the WooCommerce Contributing guideline?
Does your code follow the WordPress' 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:

Reducing initial inbox messages to only display 5 messages, and adding the "show more" button to load another 10 messages at a time.

Closes #34927 .

How to test the changes in this Pull Request:

Checkout branch
Ensure you have more than 15 inbox notes in your store (use WCA Test Helper to create more if need be)
Go to Homescreen.
You should only see 5 notes, with the "Show older" button at the bottom.
Click the button, and it should load 10 more.
The button should still remain at the bottom (if you have enough notes) and it should now read "Show more."
Dismiss any note, and it should disappear, loading one more at the end of the list.
Hit the "undo" button after dismissing a note, and it should reappear.
The button and Footer section should disappear after all notes have been loaded.

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 run tests with your changes locally?
Have you created a changelog file for each project being changed, ie pnpm --filter=<project> run changelog add?

FOR PR REVIEWER ONLY:

I have reviewed that everything is sanitized/escaped appropriately for any SQL or XSS injection possibilities. I made sure Linting is not ignored or disabled.

github-actions · 2022-10-07T22:56:40Z

Test Results Summary

Commit SHA: 1a9ba6e

Test 🧪	Passed ✅	Failed 🚨	Broken 🚧	Skipped ⏭️	Unknown ❔	Total 📊	Duration ⏱️
API Tests	188	0	0	2	0	190	0m 56s
E2E Tests	63	4	2	4	0	73	8m 8s

⚠️ Warning

Please address the following issues prior to merging this pull request:

FAILED/BROKEN TESTS. There were failed and/or broken API and E2E tests.
INCOMPLETE E2E TEST RUN. We have a total of 189 E2E tests, but only 73 were executed. Note that in CI, E2E tests automatically end when they encounter too many failures.

To view the full API test report, click here.
To view the full E2E test report, click here.
To view all test reports, visit the WooCommerce Test Reports Dashboard.

joelclimbsthings · 2022-10-07T23:42:35Z

plugins/woocommerce-admin/client/inbox-panel/index.js

+		DEFAULT_INBOX_QUERY.per_page
+	);
+	const [ allNotesFetched, setAllNotesFetched ] = useState( false );
+	const [ allNotes, setAllNotes ] = useState( [] );


This is used to store fetched notes, since we don't want them all to disappear (as happens with the returned notes variable with useSelect) while fetching more notes. This results in a much better UI experience.

plugins/woocommerce-admin/client/inbox-panel/index.js

joelclimbsthings · 2022-10-07T23:44:30Z

plugins/woocommerce-admin/client/inbox-panel/index.js

@@ -238,15 +299,17 @@ const InboxPanel = ( { showHeader = true } ) => {

 		const noteId = note.id;
 		try {
-			removeNote( noteId );
+			await removeNote( noteId );
+			invalidateResolutionForStoreSelector( 'getNotes' );


It seems we need to invalidate the data selector manually; I'm assuming this is due to the the fact that we're changing the inbox query with each request, but I haven't gotten too deep into this. Let me know if you see a more elegant way to manage this.

Yeah that makes sense, I don't think there is a much better way around this at the moment, unless we add a new selector or change the selector to either ignore per_page prop when returning the items, or use a separate selector that just returns all the notes.
But this is fine!

I think this will result in the simplest way to give the "rolling" notes appearance as we dismiss/etc notes. I'll keep it as is for the moment.

louwie17

Thanks for working on this @joelclimbsthings, this tested well. I did leave several comments within the code, not all of my suggestions are a must.
Given this would need to be in for the code freeze, the biggest item for me is having to use the notesHash thing, if we could remove that then I would be happy.
I was wondering what exact scenarios you needed the hash for?
And curious if just removing the map call within the useSelect will remove the need of having to use the hash?

plugins/woocommerce-admin/client/inbox-panel/index.js

louwie17 · 2022-10-12T20:21:46Z

plugins/woocommerce-admin/client/inbox-panel/index.js

+									quantity_shown: allNotes.length,
+								} );
+								setNoteDisplayQty(
+									noteDisplayQty + ADD_NOTES_AMOUNT


Instead of relying on the noteDisplayQty state to re-trigger the useSelect hook, would it make more sense to remove the useSelect hook and use resolveSelect instead?
This allows you to run the getNotes selector just like apiFetch and it returns a promise, like so:
resolveSelect( NOTES_STORE_NAME ).getNotes( inboxQuery ).then( (notes) => setAllNotes(notes)

I am not opposed to the useSelect but given the extra functionality I wonder if useSelect would help simplify some of the logic, especially around when to set the all notes state.

I've greatly simplified the useSelect() in the last commit, so I don't believe we'd see much benefit from this change. I haven't used resolveSelect(), so I appreciate the insight!

louwie17 · 2022-10-12T20:25:50Z

plugins/woocommerce-admin/client/inbox-panel/index.js

@@ -238,15 +299,17 @@ const InboxPanel = ( { showHeader = true } ) => {

 		const noteId = note.id;
 		try {
-			removeNote( noteId );
+			await removeNote( noteId );
+			invalidateResolutionForStoreSelector( 'getNotes' );


Yeah that makes sense, I don't think there is a much better way around this at the moment, unless we add a new selector or change the selector to either ignore per_page prop when returning the items, or use a separate selector that just returns all the notes.
But this is fine!

plugins/woocommerce-admin/client/inbox-panel/index.js

louwie17

One minor suggestion for the lint error, but otherwise this looks great and tested well!

louwie17 · 2022-10-12T21:55:38Z

plugins/woocommerce-admin/client/inbox-panel/index.js

@@ -142,59 +178,78 @@ const renderNotes = ( {
 					);
 				} ) }
 			</TransitionGroup>
+			{ allNotesFetched ? null : notesHaveResolved ? (


Nested ternary expressions is not like by lint.
Maybe splitting this into two separate conditions would be the best?

Or could we show the InboxNotePlaceholder when allNotesFetched is false?

Hmm, not sure how I feel about that particular rule, but I've remedied this in 9516508.

plugins/woocommerce-admin/client/inbox-panel/index.js

louwie17

Thanks for working on this @joelclimbsthings, this looks good and it tested well, nice work!!

… hash

louwie17

Thanks for fixing the test Joel, and I checked the E2E tests, they do not seem to be related. I saw the same one's fail in several other PRs.
I will merge this for you.

github-actions · 2022-10-13T11:36:18Z

Hi @louwie17, thanks for merging this pull request. Please take a look at these follow-up tasks you may need to perform:

Add the release: add testing instructions label

github-actions bot added focus: react admin plugin: woocommerce Issues related to the WooCommerce Core plugin. release: highlight Issues that have a high user impact and need to be discussed/paid attention to. labels Oct 7, 2022

joelclimbsthings marked this pull request as draft October 7, 2022 22:40

joelclimbsthings commented Oct 7, 2022

View reviewed changes

plugins/woocommerce-admin/client/inbox-panel/index.js Outdated Show resolved Hide resolved

joelclimbsthings commented Oct 7, 2022

View reviewed changes

joelclimbsthings self-assigned this Oct 7, 2022

joelclimbsthings requested a review from a team October 7, 2022 23:45

joelclimbsthings marked this pull request as ready for review October 7, 2022 23:46

AnnaMag added the focus: inbox label Oct 12, 2022

joelclimbsthings force-pushed the update/34927-inbox-expansion branch from 654dfe7 to a0bcf7a Compare October 12, 2022 18:52

github-actions bot removed the release: highlight Issues that have a high user impact and need to be discussed/paid attention to. label Oct 12, 2022

louwie17 requested changes Oct 12, 2022

View reviewed changes

joelclimbsthings force-pushed the update/34927-inbox-expansion branch from a0bcf7a to 47effe8 Compare October 12, 2022 20:48

joelclimbsthings requested a review from louwie17 October 12, 2022 21:35

louwie17 reviewed Oct 12, 2022

View reviewed changes

louwie17 previously approved these changes Oct 12, 2022

View reviewed changes

joelclimbsthings dismissed louwie17’s stale review via 2d37a8b October 12, 2022 23:39

joelclimbsthings added 10 commits October 12, 2022 20:03

Basic paging functionality

5508cc0

Adding changelog

4447068

Inserting placeholder at end of list, working dismissal

bb11077

Fixing issues with dismissed notes not updating UI

67e1fe2

Fixing odd display when removing last note

d3baafa

Renaming page variable

30d7b7d

Adding tracks event

6c2fe8e

Addressing issues caused by .map within useSelect, and getting rid of…

b7f7f39

… hash

Fixing nested ternary linting issue

c421b32

Removing console logs

5f58778

Updating test

1a9ba6e

joelclimbsthings force-pushed the update/34927-inbox-expansion branch from 2d37a8b to 1a9ba6e Compare October 13, 2022 03:04

AnnaMag added this to the 7.1.0 milestone Oct 13, 2022

louwie17 approved these changes Oct 13, 2022

View reviewed changes

louwie17 merged commit 315e163 into trunk Oct 13, 2022

louwie17 deleted the update/34927-inbox-expansion branch October 13, 2022 11:36

louwie17 added the release: add testing instructions PRs that have not had testing instructions added to the wiki. [auto] label Oct 13, 2022

github-actions bot mentioned this pull request Nov 10, 2022

Test WooCommerce 7.1 UVLabs/location-picker-at-checkout-for-woocommerce#55

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only show five inbox messages and add "Show more" button #35003

Only show five inbox messages and add "Show more" button #35003

joelclimbsthings commented Oct 7, 2022

github-actions bot commented Oct 7, 2022 •

edited

joelclimbsthings Oct 7, 2022

joelclimbsthings Oct 7, 2022 •

edited

louwie17 Oct 12, 2022

joelclimbsthings Oct 12, 2022

louwie17 left a comment

louwie17 Oct 12, 2022

joelclimbsthings Oct 12, 2022

louwie17 Oct 12, 2022

louwie17 left a comment

louwie17 Oct 12, 2022

joelclimbsthings Oct 12, 2022

louwie17 left a comment

louwie17 left a comment

github-actions bot commented Oct 13, 2022 •

edited by louwie17

Only show five inbox messages and add "Show more" button #35003

Only show five inbox messages and add "Show more" button #35003

Conversation

joelclimbsthings commented Oct 7, 2022

All Submissions:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Other information:

FOR PR REVIEWER ONLY:

github-actions bot commented Oct 7, 2022 • edited

Test Results Summary

⚠️ Warning

Choose a reason for hiding this comment

joelclimbsthings Oct 7, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

louwie17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

louwie17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

louwie17 left a comment

Choose a reason for hiding this comment

louwie17 left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 13, 2022 • edited by louwie17

github-actions bot commented Oct 7, 2022 •

edited

joelclimbsthings Oct 7, 2022 •

edited

github-actions bot commented Oct 13, 2022 •

edited by louwie17