-
Notifications
You must be signed in to change notification settings - Fork 5
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
Make content filters dynamic #583
Conversation
Contribute to #573
✅ Deploy Preview for veda-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
thematics: | ||
- covid-19 | ||
- agriculture |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thematics
can probably be removed now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. this will need to move under taxonomy.Topics
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me, this solution fulfils the two goals we had:
- Presenting the complete taxonomy (as drop-downs) to the user for a structured overview of the catalog
- Enable users to quickly find the data they are looking for, by using one of several possible filters, or a combination.
Just for the record - we discussed previously that a possible improvement would be that only combinations of filters are shown that actually exist, such that users do not end up with no results and instead can use the filters to refine their search step-wise. We can discuss that as a future improvement - or table it until users perhaps request this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on it.
Now I realize that the validation for taxonomies will be difficult since we won't know if it is a typo or just a similar text. But this approach still makes sense under the current Veda condition I think. At least editor can see what they did wrong through the UI quickly and can act on it.
A small incoherent behavior I noticed is when Topic is selected, the URL shows not-encoded query parameters while showing the encoded URL for other taxonomy options.
+(This function will definitely benefit greatly from some autocomplete UI.)
@@ -12,6 +12,16 @@ media: | |||
thematics: | |||
- covid-19 | |||
- agriculture | |||
taxonomy: | |||
Topics: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes me wonder if users will understand what they type here is what they will see. (Since this is the only capitalized attribute in mdx, it looks wrong somehow in mdx context.) But I also don't really see a better solution since there will be various requirements for capitalization 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree capitalized keys (perhaps also with special characters and spaces) could perhaps be avoided, to keep YAML keys clean?
Is this better?
taxonomy:
- title: Topic
values:
- Covid 19
- Agriculture
- title: Sector
values:
- Electricity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, this is a very valid concern. In this way it perhaps increases clarity with the cost of verbosity (in an already complex frontmatter). Not sure which one is preferable.
@hanbyul-here The filters get encoded as json deliberately. It is not possible to create individual keys for each taxonomy, since we don't know how many there are. Each key is created with a hook, and dynamically creating them would mean to have a loop creating the hooks, which is not ok. Therefore, all taxonomy options are stored as an object under taxonomy
which get encoded as json. We could write a custom hydrate|dehydrate
function to make it prettier, but I'm wondering if it is worth it. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, my point was more about
When you click 'Ccovid 19' from the card, the URL changes to: http://localhost:9000/discoveries?taxonomy={%22Topics%22:%22ccovid-19%22}
but when you select 'Ccovid 19' from the dropdown, the URL changes to: http://localhost:9000/discoveries?taxonomy=%7B%22Topics%22%3A%22ccovid-19%22%7D
I think it is because JSON.stringify doesn't encode {}
? I don't think it is a blocker but thought you might have missed it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hanbyul-here I had completely misunderstood what you meant. 😅 Good catch. Fixed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yay thanks for handling. I like @j08lue 's idea about making the value explicit. It is a valid concern that it will be verbose, but I feel like that is more coherent with other attributes of metadata too.
25b1bf6
to
b3bfa3d
Compare
Fixed the encoding error, added a metadata section on content pages, and updated documentation. Content migration guide to add to the release: Content migrationTaxonomyWith the removal of Example: name: Dataset Name
thematics:
- covid-19
- agriculture
sources:
- dev-seed becomes name: Dataset Name
taxonomy:
- name: Topics
values:
- Covid 19
- Agriculture
- name: Source
values:
- Development Seed Taxonomy IndexThe taxonomy index file is no longer required and can be removed. |
4ffa9a8
to
52f9d37
Compare
@hanbyul-here @j08lue I updated the implementation and documentation to use the more verbose array format. @karitotp Do you have some time to do a test of this dynamic filter feature and ensure everything is working properly? |
@danielfdsilva, the dynamic filters are working well on the Data Catalog and Data Stories. Just to make sure, the filter by search field on both pages, just works when the input text is in the title or the description of the cards as the placeholder says, right? I saw that in the previous versions, it applied to the pill names too, but currently, it does not, and it looks like this. |
As @karitotp mentioned, the recent commits seem to change how text filter works? |
@karitotp @hanbyul-here That was a bug and was not supposed to happen. It's fixed now |
This PR makes the filters on the data catalog and data stories pages dynamic, using the Automatically inferred approach described in #573.
The topics and source properties were moved to
taxonomy
, to be able to have them as filters. Currently these are also used to display information on the cards.The behavior of the filters is limited to selecting one option from a list. It is not possible to create boolean filters (eg: dataset has property) or even more complex filters.
Given that the filters need to scale they now stack, although there should be some common sense when creating new taxonomies as to not clutter the interface.
TODO: