@broskoski => Schema for followed artists filter #348
Conversation
|
Gravity PR's: https://github.com/artsy/gravity/pull/10200 (filtering by artists you follow), and https://github.com/artsy/gravity/pull/10203 (return counts for followed artists in aggregations) |
|
Looks good but ❌ |
|
💚 |
| followed_artists_total: { | ||
| type: GraphQLInt, | ||
| resolve: ({ aggregations }) => aggregations.followed_artists.value, | ||
| }, |
There was a problem hiding this comment.
Shouldn’t these counts go in a counts object?
There was a problem hiding this comment.
Well, the current total we just expose in the schema, so this just follows that pattern.
There was a problem hiding this comment.
Yeah I get that, but I guess that makes sense when there’s only 1 count field. I’m more wondering in general when the best time would be to add a counts object. Should we do that even if there’s only 1 count field so that it’s clear where subsequent count fields need to go and we don’t need to move API around?
There was a problem hiding this comment.
Hmm, not sure the best practices around counts field, I haven't used them yet.
I'm happy to do whatever, since there is already deployed code that depends on the current schema (as well as a pending Force PR: artsy/force#5344, mind making that an issue that I can tackle later?
There was a problem hiding this comment.
Yeah, in general I think we want to stick to the counts convention that Master @dzucconi set up, so its always one way. Feel free to follow up in a future PR if it's more complicated to change now though.
There was a problem hiding this comment.
Cool, yea let's do it in a subsequent PR, esp since the Force one is merged.
There was a problem hiding this comment.
Ah this one is merged. Cool! 👍
There was a problem hiding this comment.
@mzikherman Filed a ticket #350
|
Bump. |
mzikherman
force-pushed
the
commercial_filtering_followed_artists
branch
from
a93b450
to
fabff66
Compare
Needed for Comm filtering 1.6 (artworks by followed artists filter)