Skip to content
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.

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

Dashboard: support filtering stories by taxonomy #11625

Merged
merged 66 commits into from
Jun 13, 2022

Conversation

mwritter
Copy link
Contributor

@mwritter mwritter commented Jun 2, 2022

Context

Add programmatic taxonomy filters to the dashboard.

Summary

Give the user the ability to filter by story taxonomies. Similar to the author filter.

Relevant Technical Choices

There can be multiple taxonomy filters, and a programmatic solution is needed to build out these filters. The solution used here lends itself to encompass all dashboard filters state into one place making the filtering process simpler later on.

To-do

Move all dashboard story filters into the new provider to simplify dashboard story filtering

User-facing changes

before/after
https://user-images.githubusercontent.com/14057928/171685426-33d5664b-0485-4567-8773-d45d6aae5bf8.mp4

Testing Instructions

  • This is a non-user-facing change and requires no QA

This PR can be tested by following these steps:

  1. Go to story dashboard

On Staging:

  • Notice there are no taxonomy filters

On Branch:

  • Notice there are taxonomy filters (categories, verticals)
  1. Make sure there are stories with taxonomies assigned to them
  2. Test filters correctly filters stories
  3. Test that the search within the filter dropdown is working correctly as well

Reviews

Does this PR have a security-related impact?

No

Does this PR change what data or activity we track or use?

No

Does this PR have a legal-related impact?

No

Checklist

  • This PR addresses an existing issue and I have linked this PR to it in ZenHub
  • I have tested this code to the best of my abilities
  • I have verified accessibility to the best of my abilities (docs)
  • I have verified i18n and l10n (translation, right-to-left layout) to the best of my abilities
  • This code is covered by automated tests (unit, integration, and/or e2e) to verify it works as intended (docs)
  • I have added documentation where necessary
  • I have added a matching Type: XYZ label to the PR

Fixes #11436

@swissspidy
Copy link
Collaborator

@sblinde In short, let's proceed.

But let me just take a step back, ignore preloading, PHP, etc. to describe the issue at hand.

When the dashboard loads, the authors dropdown is already visible, just with a "No authors found" message until all authors have been loaded:

Screenshot 2022-06-10 at 13 47 00

On the other hand, the taxonomy dropdowns are only displayed after all the terms have been loaded (getTaxonomyTerms), even though the taxonomies themselves (getTaxonomies) have been loaded.

Note: just to clarify, the getTaxonomies call is in fact already preloaded, so there's no HTTP request we're waiting for there.

Screen.Recording.2022-06-10.at.13.42.52.mov

The question is:

Is it possible to display the dropdowns before all the terms have been loaded, just with a "No xyz found" message?

From what I can see, yes, it is possible.

The issue is in the queryTaxonomies callback within useTaxonomyFilters():

/**
* Query all the taxonomies.
* This should only be needed once get all the taxonomies and set individual term data.
*
* @see queryTaxonomyTerms
*/
const queryTaxonomies = useCallback(async () => {
const data = await getTaxonomies({ hierarchical: true });
const promises = [];
for (const taxonomy of data) {
// initialize the data with an empty array
taxonomy.data = [];
promises.push(queryTaxonomyTerms(taxonomy));
}
const fetched = await Promise.all(promises);
for (const arr of fetched) {
// grab the first elements 'taxonomy' to map the array of terms back to the parent taxonomy
const key = arr.at(0)?.taxonomy;
if (key) {
const taxonomy = data.find((h) => h.slug === key);
if (taxonomy) {
taxonomy.data = arr;
}
}
}
setTaxonomies(data);
}, [setTaxonomies, getTaxonomies, queryTaxonomyTerms]);

setTaxonomies() is only ever called after all the terms have been loaded.

A quick hack locally fixed this (though the terms loading seemed to be broken afterwards):

  const queryTaxonomies = useCallback(async () => {
    const data = await getTaxonomies({ hierarchical: true });
+   setTaxonomies(data);
    const promises = [];

Proof:

Screen.Recording.2022-06-10.at.14.09.32.mov

It seems to me that queryTaxonomies can also be optimized a bit to not wait for all terms to be loaded before updating the state. Instead, the state can be updated multiple times, after each successful loading.

@spacedmonkey
Copy link
Contributor

While testing this PR, I found a related issue -#11668.

@sblinde
Copy link
Contributor

sblinde commented Jun 10, 2022

@swissspidy Thank you for all of the details you shared and feedback there. I see what you mean, and I totally agree with your assessment: those dropdowns should ideally be available prior to the terms loading, and as you showed, if we're setting taxonomies only after they're fetched, there's the reason. Since we have that epic slated for improvements to filtering, we'll make sure we address that. Thanks for the video examples too.

We'll work on getting this across the finish line today, and filtering 2.0 should be able to encompass more work to make this experience a better one.

@spacedmonkey Nice find! I added it to the epic for improvements to filtering as well.

Again, thank you both for the feedback through all of this work. We appreciate it.

Copy link
Contributor

@sblinde sblinde left a comment

Choose a reason for hiding this comment

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

👏👏👏👏

Well done. Gave the code a quick check again, and did a quick QA on the qa instance. Everything at first glance is working well so far (w/ just categories and categories + custom taxonomies).

@spacedmonkey
Copy link
Contributor

Screenshot 2022-06-13 at 11 05 56

What should happen in if there are lots of taxonomies registered? How should it stack / clip?

@spacedmonkey
Copy link
Contributor

Created a little PR to ensure that only taxnomies that show_ui flag is set to true - #11678

@mwritter
Copy link
Contributor Author

What should happen in if there are lots of taxonomies registered? How should it stack / clip?

I believe we need some design input on this one @sblinde.

@sblinde
Copy link
Contributor

sblinde commented Jun 13, 2022

What should happen in if there are lots of taxonomies registered? How should it stack / clip?

I believe we need some design input on this one @sblinde.

@mwritter Agreed! I'll make sure it's part of the improvements epic ✅

Update: Added to #11665

@swissspidy swissspidy merged commit 8f8319b into main Jun 13, 2022
@swissspidy swissspidy deleted the feature/11436-Filter-By-Taxonomy branch June 13, 2022 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: Dashboard /packages/dashboard Type: Enhancement New feature or improvement of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dashboard: Add Filter by Taxonomy
5 participants