8000 Fix infinite category loading state by octaedro · Pull Request #40073 · woocommerce/woocommerce · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix infinite category loading state #40073

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Sep 8, 2023
Merged

Fix infinite category loading state #40073

merged 3 commits into from
Sep 8, 2023

Conversation

octaedro
Copy link
Contributor
@octaedro octaedro commented Sep 7, 2023 •
edited
Loading

Submission Review Guidelines:

Changes proposed in this Pull Request:

After merging this PR (which sends the isLoading prop to the SelectTreeMenu) and this PR, the category selector under Products > Add New > Organization never loses the loading state.

The problem is that the hook useTaxonomySearch never sets the isLoading state to false after fetching.

This PR fixes the problem.

How to test the changes in this Pull Request:

Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:

  1. Enable the product blocks editor under WooCommerce -> Settings -> Advanced -> Features.
  2. Navigate to Products > Add new.
  3. Press Organization and select the Categories dropdown menu.
  4. The loading spinner should disappear after a few seconds, and it should show the categories available.

Changelog entry

  • Automatically create a changelog entry from the details below.

Significance

  • Patch
  • Minor
  • Major

Type

  • Fix - Fixes an existing bug
  • Add - Adds functionality
  • Update - Update existing functionality
  • Dev - Development related task
  • Tweak - A minor adjustment to the codebase
  • Performance - Address performance issues
  • Enhancement - Improvement to existing functionality

Message

Comment

@octaedro octaedro requested a review from a team September 7, 2023 15:57
@octaedro octaedro self-assigned this Sep 7, 2023
@github-actions
Copy link
Contributor
github-actions bot commented Sep 7, 2023

Hi , @woocommerce/mothra

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

@octaedro octaedro added Bug The issue is a confirmed bug. priority: high labels Sep 7, 2023
@octaedro octaedro marked this pull request as ready for review September 7, 2023 15:58
@github-actions
Copy link
Contributor
github-actions bot commented Sep 7, 2023 •
edited
Loading

Test Results Summary

Commit SHA: 906bdbc

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests25900202611m 2s
E2E Tests1950015021021m 46s
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.

mattsherman
mattsherman approved these changes Sep 7, 2023
View reviewed changes
Copy link
Contributor
@mattsherman mattsherman 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 quick fix!

packages/js/product-editor/src/blocks/taxonomy/use-taxonomy-search.ts Outdated
Comment on lines 72 to 75
setIsSearching( false );
} catch ( e ) {
setIsSearching( false );
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This works. One possible alternative would be to put it in a finally block and change the catch to show an alert that an error occurred...

} finally {
    setIsSearching( false );
}

But, I'm okay with keeping things exactly as you have them for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call! I changed that in the commit 906bdbc

@octaedro octaedro merged commit 48016ef into trunk Sep 8, 2023
@octaedro octaedro deleted the fix/infinite_category_loading_state branch September 8, 2023 11:58
@github-actions github-actions bot added this to the 8.2.0 milestone Sep 8, 2023
@github-actions github-actions bot added the needs: analysis Indicates if the PR requires a PR testing scrub session. label Sep 8, 2023
@nigeljamesstevenson nigeljamesstevenson added needs: external testing Indicates if the PR requires further testing conducted by testers external to the development team. and removed needs: analysis Indicates if the PR requires a PR testing scrub session. labels Sep 8, 2023
@nigeljamesstevenson nigeljamesstevenson added the status: analysis complete Indicates if a PR has been analysed by Solaris label Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattsherman mattsherman mattsherman approved these changes

Assignees

@octaedro octaedro

Labels
Bug The issue is a confirmed bug. needs: external testing Indicates if the PR requires further testing conducted by testers external to the development team. status: analysis complete Indicates if a PR has been analysed by Solaris
Projects
None yet
Milestone
8.2.0
Development

Successfully merging this pull request may close these issues.
3 participants
@octaedro @mattsherman @nigeljamesstevenson
0