noi-techpark / it.bz.opendatahub.databrowser

Explore and navigate through Open Data you need to build your next service.
https://databrowser.opendatahub.com
GNU Affero General Public License v3.0
9 stars 7 forks source link

Feat/new filters menu in Datasets Overview page #420

Closed MatteoBiasi closed 1 year ago

gappc commented 1 year ago

Hey @MatteoBiasi, thank you for this PR, looks good overall and i like it :+1:

Please don't get scared because of the number of review comments (especially on OverviewListPage.vue), most are best-practices I wanted to share like typing. Note, that I didn't mark all code parts, e.g. I didn't mark all code parts where types could be improved. Please check for your self where comments apply.

Testing the feature, it seems to run fine, I encountered just one strange behavior: if I set filters of the same type, they seem to work in a logical AND fashion instead of the expected OR. In the image below, I expected to see the datasets that belong to tourism OR mobility when I select both Dataspace filters. Instead, the result is empty, which seems odd.

image

MatteoBiasi commented 1 year ago

Hey @MatteoBiasi, thank you for this PR, looks good overall and i like it 👍

Please don't get scared because of the number of review comments (especially on OverviewListPage.vue), most are best-practices I wanted to share like typing. Note, that I didn't mark all code parts, e.g. I didn't mark all code parts where types could be improved. Please check for your self where comments apply.

Testing the feature, it seems to run fine, I encountered just one strange behavior: if I set filters of the same type, they seem to work in a logical AND fashion instead of the expected OR. In the image below, I expected to see the datasets that belong to tourism OR mobility when I select both Dataspace filters. Instead, the result is empty, which seems odd.

image

Hi @gappc, thanks for the review, but I cannot see the comments. 😅 This is the only comment I see. Can you double check if you can see them?

For the second part of the message, I had the same doubt. At the moment the frontend always uses an AND and never an OR. This is more an UX issue than coding: by my point of view, it makes sense for some filters to act as an AND; for example for tags, you may want all Datasets which match 2 or more tags. But for some others, like the Dataspace, it totally doesn't make sense since the Dataspace can only be a or b, and not have both. We may generalize and say that all filters which filter on a parameter which is an Array, should use the AND logic, while those who match on a String, should use the OR logic. But then at the same time we need to improve the UI showing for example rounded checkboxes (which will remind the input radio) were it's only possibile to choose a value and keep the squared checkboxes where it's possibile to choose multiple values. This is something that definitively has to be discussed. I quote @mrabans so maybe we can discuss it in the next meeting.

gappc commented 1 year ago

@MatteoBiasi strange, I can see them as long as I'm logged in (if I'm logged out I can't see them either). Maybe I have to finish the review, just a sec...

MatteoBiasi commented 1 year ago

@MatteoBiasi strange, I can see them as long as I'm logged in (if I'm logged out I can't see them either). Maybe I have to finish the review, just a sec...

@gappc yep that was the issue. Now I can see them. 😊

mrabans commented 1 year ago

This is something that definitively has to be discussed. I quote @mrabans so maybe we can discuss it in the next meeting.

@MatteoBiasi absolutely, bring the topic up on Tuesday, so we can discuss it.

MatteoBiasi commented 1 year ago

@gappc All PR issues have been fixed (check my only comment), the filtering logic has been optimised handling and/or between macro filters groups and group options. Everything should be ready to me merged. 😊

gappc commented 1 year ago

@MatteoBiasi thank you for the changes. The only blocker is the remaining type import for lodash, the other stuff is optional

MatteoBiasi commented 1 year ago

@gappc the latest comments have been solved. Should be ready to merge. 👍

gappc commented 1 year ago

@MatteoBiasi nice work, thank you :)