Skip to content

Conversation

@joshuatf
Copy link
Contributor

@joshuatf joshuatf commented Oct 28, 2022

All Submissions:

Changes proposed in this Pull Request:

  • Adds the option to filter notes in the REST endpoint using is_read
  • Adds the option to filter notes by is_read in the data store
  • Uses the unread notes count in the inbox panel header
  • Adds logic to mark notes as read when viewing a note

@pmcpinto @jarekmorawski This PR marks notes as read when viewing them. One issue with this is that a note is instantly marked as read and the unread indicator flashes off the screen almost immediately. It might be worth adding a delay before this occurs or caching the notes read status temporarily to avoid losing the unread indicator immediately.

Also cc @moon0326 to make sure I'm not creating regressions with the existing logic. It seems like a choice was made in woocommerce/woocommerce-admin#6047 and woocommerce/woocommerce-admin#7896 to only mark notes as read when they were actioned. This obviously undoes some of that work, but we are unlikely to see read notes if they are not actioned.

Screen Shot 2022-10-28 at 12 44 52 PM

Closes #34929 .

  • This PR is a very minor change/addition and does not require testing instructions (if checked you can ignore/remove the next section).

How to test the changes in this Pull Request:

  1. Create a site with at least 1 or more unread notes
  2. Visit the home page and scroll down to see the notes
  3. Note that unread notes are marked as read on viewing
  4. Note that the count of notes in the inbox panel header reflects unread notes (all unread notes, not just the visible ones prior to loading the remaining notes).

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 created a changelog file for each project being changed, ie pnpm --filter=<project> 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.

@joshuatf joshuatf requested review from a team and moon0326 October 28, 2022 19:45
@joshuatf joshuatf self-assigned this Oct 28, 2022
@github-actions github-actions bot added focus: react admin package: @woocommerce/data issues related to @woocommerce/data plugin: woocommerce Issues related to the WooCommerce Core plugin. labels Oct 28, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Oct 28, 2022

Test Results Summary

Commit SHA: ae6d71e

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests26000202620m 51s
E2E Tests186006019213m 31s

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.

Copy link
Contributor

@moon0326 moon0326 left a comment

Choose a reason for hiding this comment

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

Thank you for working on it @joshuatf

Changes LGTM, but I'm getting the following error. I've looked into it briefly, but I don't see any changes causing the error 🤔 It might be just my local.

Screen Shot 2022-10-31 at 2 49 45 PM

Screen Shot 2022-10-31 at 2 51 24 PM

@joshuatf
Copy link
Contributor Author

joshuatf commented Nov 1, 2022

Hey @moon0326 thanks for the review!

When I checked out this branch again, I also saw that error, but after refreshing I was not able to reproduce the error. I added a check in aa75c46 but I'm not able to reproduce. Could you try this and see if it works for you?

@github-actions github-actions bot added the package: @woocommerce/experimental issues related to @woocommerce/experimental label Nov 1, 2022
@pmcpinto
Copy link

pmcpinto commented Nov 3, 2022

One issue with this is that a note is instantly marked as read and the unread indicator flashes off the screen almost immediately. It might be worth adding a delay before this occurs or caching the notes read status temporarily to avoid losing the unread indicator immediately.

I think that adding a delay makes sense.

@joshuatf
Copy link
Contributor Author

@pmcpinto @jarekmorawski I added a 3 second delay to this. After a note comes into view, it will be marked as read after a few seconds so that the user can continue to see the "unread" indicator when it is first scrolled into review.

Please let me know if you want to tweak that time at all.

@moon0326 This is ready for review when you have time.

@pmcpinto
Copy link

@pmcpinto @jarekmorawski I added a 3 second delay to this. After a note comes into view, it will be marked as read after a few seconds so that the user can continue to see the "unread" indicator when it is first scrolled into review.

Sounds good to me. Thanks!

Copy link
Contributor

@moon0326 moon0326 left a comment

Choose a reason for hiding this comment

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

Thanks for the fix @joshuatf 👍

I don't see the errors anymore.

🚀

@joshuatf joshuatf merged commit 527249f into trunk Nov 23, 2022
@joshuatf joshuatf deleted the fix/34929 branch November 23, 2022 16:52
@github-actions github-actions bot added this to the 7.3.0 milestone Nov 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

package: @woocommerce/data issues related to @woocommerce/data package: @woocommerce/experimental issues related to @woocommerce/experimental plugin: woocommerce Issues related to the WooCommerce Core plugin.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Change order count logic and copy to focus on new messages

4 participants