minicomp / wax-facets

The second Wax theme. Good times.
https://minicomp.github.io/wax-facets/
Other
9 stars 8 forks source link

Filters break when an item has a null value #23

Closed quinnanya closed 2 years ago

quinnanya commented 2 years ago

I'm getting this error: Liquid error (/Users/qad/Documents/GitHub/wax-facets/_includes/collection_gallery.html line 26): Cannot sort a null object. included (Liquid::ArgumentError)

when I modify the line on the browse page to be:

{% include collection_gallery.html collection='brueghel' facet_by='object_type|genre|tags|location_city' num_column=4 %}

Most of the objects have values for those facets, but not all do. If I fill in "N/A" or something to make the facet work, it'll look bad on the item page because the labels only show up when there's a value (which is convenient for hiding labels where there's no data).

elotroalex commented 2 years ago

This one looks like we have two hard choices. Either remove the sort liquid command in the collection_gallery.html file; OR, fill the data with N/A or None, or whatever. @amzoss what do you think? Should we remove it globally? Or let people know?

elotroalex commented 2 years ago

@quinnanya it looks like this is a liquid problem, not a Wax/Facets problem. One possible solution could be for us to declare some keyword to be a special value we can filter for. It could be "na" or something like that. Then everytime we encounter that we don't list it. Quinn, correct me if I'm wrong, but the goal here is to have empty values NOT show up on the facet menu, right?

quinnanya commented 2 years ago

Right, both have empty values not show up in facet AND not have any filler value trigger the label for that facet to show up when you look at the item itself.


From: Alex Gil @.> Sent: Saturday, September 11, 2021 12:54:21 PM To: minicomp/wax-facets @.> Cc: Quinn Dombrowski @.>; Mention @.> Subject: Re: [minicomp/wax-facets] Filters break when an item has a null value (#23)

@quinnanyahttps://github.com/quinnanya it looks like this is a liquid problem, not a Wax/Facets problem. One possible solution could be for us to declare some keyword to be a special value we can filter for. It could be "na" or something like that. Then everytime we encounter that we don't list it. Quinn, correct me if I'm wrong, but the goal here is to have empty values NOT show up on the facet menu, right?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/minicomp/wax-facets/issues/23#issuecomment-917468476, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AAFN2UCVYJQFJ5ZC4QTUUF3UBOXW3ANCNFSM5D2P2VHA. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

elotroalex commented 2 years ago

Got it!

amzoss commented 2 years ago

Okay, question for @quinnanya - what does the .md for the object look like? for example, if an object has a null in object_type, does the object markdown file have:

  1. no line for object_type
  2. object_type:
  3. object_type: (space at the end)
  4. object_type: ''
  5. something else?
amzoss commented 2 years ago

Basically, I'm having trouble reproducing this, and I'm sort of surprised this is happening if there's just a few null values but some objects do have values. The line in question ({% assign options = collection | map: item | compact | uniq | sort_natural %}) uses "compact" to get rid of nil values. Are you sure there isn't a field in there that has absolutely no objects with data? Like, maybe there is a typo in the field name?

amzoss commented 2 years ago

(Actually, I can't even get the build to fail with a typo now. Maybe that's not what we want, but at least the page loads for troubleshooting!)

quinnanya commented 2 years ago

It looks like I have one Problem File, at least w/ the genre and object type filters, and it's type 3 (with a space after the label): https://raw.githubusercontent.com/quinnanya/wax-documentation/master/_brueghel/18635.md

amzoss commented 2 years ago

Okay okay, so, I've downloaded your repo and I've been trying various things. I think I have an answer, but it's not very satisfying.

As far as I can tell, it's not a problem with the markdown file or our include code. Your repo works fine if you facet on any combination of object_type, genre, and location_city, even with the null values. It only breaks when you add "tags" to facet_by, and that breaks it no matter what. It actually even breaks the original Wax include.

So I think what we're dealing with is the fact that "tags" is a core Jekyll concept/reserved word. I think having a column in your dataset called tags may always break facets. If you wanted to rename the tags column in your metadata file and rerun the Wax tasks, you could confirm this for sure. But right now, I don't think there's anything we can change in our include to get 'tags' to work. (On my machine, the repo failed to build even if the include file is completely empty, so I think Jekyll was just refusing to parse the string 'tags' in an input variable in any way.)

If anyone else has any ideas on how to... escape?... the name 'tags' in the input declaration, that'd be great! But I think it may just be better practice to recommend that people avoid using Jekyll reserved words like tags for field names.

Recommend we close this, as null values actually seem to work as expected.

elotroalex commented 2 years ago

Just a quick note to point out that we already escape “date” as “_date” so we could probably do “_tags”.

❀❀❀ On Sep 15, 2021, 8:48 PM -0400, Angela Zoss @.***>, wrote:

Okay okay, so, I've downloaded your repo and I've been trying various things. I think I have an answer, but it's not very satisfying. As far as I can tell, it's not a problem with the markdown file or our include code. Your repo works fine if you facet on any combination of object_type, genre, and location_city, even with the null values. It only breaks when you add "tags" to facet_by, and that breaks it no matter what. It actually even breaks the original Wax include. So I think what we're dealing with is the fact that "tags" is a core Jekyll concept/reserved word. I think having a column in your dataset called tags may always break facets. If you wanted to rename the tags column in your metadata file and rerun the Wax tasks, you could confirm this for sure. But right now, I don't think there's anything we can change in our include to get 'tags' to work. (On my machine, the repo failed to build even if the include file is completely empty, so I think Jekyll was just refusing to parse the string 'tags' in an input variable in any way.) If anyone else has any ideas on how to... escape?... the name 'tags' in the input declaration, that'd be great! But I think it may just be better practice to recommend that people avoid using Jekyll reserved words like tags for field names. Recommend we close this, as null values actually seem to work as expected. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android.

amzoss commented 2 years ago

Good call! Yes, that would be a good solution. Do you think that kind of recommendation would be good to add to the Wax wiki docs?

quinnanya commented 2 years ago

I think this is my fault -- the tags (which should've been _tags) were something I added because this project needed a clickable taxonomy. Happy to write something for the Wiki wax docs if other people need it too, though!

elotroalex commented 2 years ago

Don’t let me stop you! :)

❀❀❀ On Sep 16, 2021, 6:10 PM -0400, Quinn Dombrowski @.***>, wrote:

I think this is my fault -- the tags (which should've been _tags) were something I added because this project needed a clickable taxonomy. Happy to write something for the Wiki wax docs if other people need it too, though! — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android.

amzoss commented 2 years ago

I went ahead and proposed something for the docs! I think we can close this issue.

https://github.com/minicomp/wiki/pull/29