Dashboard: Incorporate the rest of the filters into FiltersProvider #11851
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +949 B (0%) Total Size: 2.65 MB
ℹ️ View Unchanged
|
|
Plugin builds for 03cc6ce are ready 🛎️!
|
|
This pull request introduces 1 alert when merging b6ea60f into 5efe289 - view on LGTM.com new alerts:
|
|
@mwritter There seems to be valid test failures here. Let's get those fixed. |
@spacedmonkey For sure, I was off for the 4th and 5th 🇺🇸 - Im looking into them 👍 |
swissspidy
reviewed
Jul 8, 2022
| @@ -82,7 +82,7 @@ describe('Publish panel in document tab', () => { | |||
| await page.keyboard.type('auth'); | |||
|
|
|||
| // wait for search results | |||
| await page.waitForTimeout(200); | |||
| await page.waitForTimeout(500); | |||
There was a problem hiding this comment.
This is being addressed in #11782
|
Not sure if this related to this PR. But I am seeing no options on search. But their are results.
|
On the QA instance, with this PR enabled, I'm not able to replicate the same that you're seeing.
|
@spacedmonkey - That search "dropdown" is populated with story titles, based on your screenshot none of those stories have titles so there are 'No options available' to select in that "dropdown". |
sblinde
reviewed
Jul 11, 2022
| expect(statusFilter.filterId).toBe('New'); | ||
| }); | ||
|
|
||
| it('should be able to update the sortObject if the acceptable keys are use', () => { |
There was a problem hiding this comment.
Nit: Probably missing a letter for used here
sblinde
reviewed
Jul 11, 2022
| expect(filter.filterId).toBeNull(); | ||
|
|
There was a problem hiding this comment.
It might be helpful to have some more comments in these tests to indicate what you're trying to test / do in each case
There was a problem hiding this comment.
Or, alternatively, separate the use cases of these tests instead of all in one
Need to wait until the bot leaves a comment saying that the PR is available for testing. Right now this doesn't happen when a draft PR is set to ready for review. It's only triggered when pushing some changes to a non-draft PR. I just merged main into this branch, so in a few minutes the bot will leave a comment and only then will this PR begin to be available for testing on the QA site. Hope that makes sense. |
…ogleForCreators/web-stories-wp into feature/11789_Incorporate_All_Filters
|
I hadn't realized that the bot didn't update its initial commenting in picking up on the latest changes, makes sense. Thanks for that! |
sblinde
approved these changes
Jul 12, 2022
Ah, my bad for not realizing that the bot did add comments earlier but then the PR was converted to a draft again, which means the ZIPs were never updated. That's indeed very confusing! I'll try to think of a good way to address that. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context
Move all filters/sorting logic into one place.
Summary
Incorporates the 'status', 'keyword search', and 'sort' in the FiltersProvider.
Relevant Technical Choices
Moving all the filters/sort into one place and using a hook to tap into them is more maintainable than prop drilling.
To-do
The way we use 'tracking' when 'searching' may need to be updated now that the logic has been moved. Possibly more refactoring to remove old/unnecessary logic.
User-facing changes
Testing Instructions
This PR can be tested by following these steps:
Reviews
Does this PR have a security-related impact?
No
Does this PR change what data or activity we track or use?
Possibly - I do believe we'll need to update the search tracking
Does this PR have a legal-related impact?
No
Checklist
Type: XYZlabel to the PRFixes #11668
Fixes #11789
Fixes #11780