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

orderBy price ascending should put prioritise "listed" tokens #188

Closed
hassnian opened this issue Jan 11, 2024 · 15 comments · Fixed by #194
Closed

orderBy price ascending should put prioritise "listed" tokens #188

hassnian opened this issue Jan 11, 2024 · 15 comments · Fixed by #194
Assignees
Labels
bug Something isn't working

Comments

@hassnian
Copy link
Contributor

hassnian commented Jan 11, 2024

          nope, not working yet 

image

when this filter is applied it should show listed first - sorted by price and then the rest

Originally posted by @exezbcz in kodadot/nft-gallery#8920 (comment)

ref kodadot/nft-gallery#8890

sort by price + buy now = listed items sorted by price

sort by price = all items sorted based on price, non listed are there as well, but only after the listed ones

@hassnian hassnian changed the title tokenEntities orderBy price not working as expected. tokenEntities orderBy price not working as expected Jan 11, 2024
@maige-app maige-app bot added the bug Something isn't working label Jan 11, 2024
@exezbcz
Copy link
Member

exezbcz commented Jan 20, 2024

👀

@vikiival
Copy link
Member

Isnt this issue of the query than the app ?

@exezbcz
Copy link
Member

exezbcz commented Jan 20, 2024

cc @hassnian

@hassnian
Copy link
Contributor Author

thread where we discussed this


query x {
  tokenEntityList(orderBy: [
    "price_ASC"
  ]) {
    cheapest {
      price
    }
  }
}

returns

{
  "data": {
    "tokenEntityList": [
      {
        "cheapest": {
          "price": "0"
        }
      },
      {
        "cheapest": {
          "price": "0"
        }
      },
      {
        "cheapest": {
          "price": "0"
        }
      }
       ....

    ]
  }
}

when this filter is applied it should show listed first - sorted by price and then the rest

shows non-listed first , but we need listed first sorted by price and then non-listed

@roiLeo
Copy link
Contributor

roiLeo commented Jan 22, 2024

query x {
tokenEntityList(orderBy: [
"price_ASC"
]) {
cheapest {
price
}
}
}

you should be able to remove "0" price

query x {
  tokenEntityList(orderBy: ["price_ASC"], limit: 10, price_gt: 0) {
    cheapest {
      price
    }
  }
}

@hassnian
Copy link
Contributor Author

hassnian commented Jan 22, 2024

query x {
tokenEntityList(orderBy: [
"price_ASC"
]) {
cheapest {
price
}
}
}

you should be able to remove "0" price

query x {
  tokenEntityList(orderBy: ["price_ASC"], limit: 10, price_gt: 0) {
    cheapest {
      price
    }
  }
}

@roiLeo yep, thought the same.

my concern was regarding pagination.

so for example, you have x pages of listed items. where you use the query above, then when there aren't more items, you show non-listed using priceEq: '0'

once we change the query first and offset which uses page are not the same as the first query

I reckon this can be done but wouldn't this add more complexity or am I missing something?

@roiLeo
Copy link
Contributor

roiLeo commented Jan 22, 2024

my concern was regarding pagination.

use offset?

so for example, you have x pages of listed items. where you use the query above, then when there aren't more items, you show non-listed using priceEq: '0'

once we change the query first and offset which uses page are not the same as the first query

Do you have a query to illustrate your example?

I reckon this can be done but wouldn't this add more complexity or am I missing something?

if it add more complexity on the frontend then it's a data issue and should be fixed on backend

@hassnian
Copy link
Contributor Author

hassnian commented Jan 22, 2024

ItemsGrid.vue -> useFetchSearch (153 line) -> useItemsGrid.ts (useFetchSearch) (141 line) we have

    const defaultSearchVariables = {
      first: first.value,
      offset: (page - 1) * first.value,
      orderBy: getRouteQueryOrderByDefault(route.query.sort, [
        'blockNumber_DESC',
      ]),
    }

to calculate the offset we use page and first which is 40

we can use this query for the listed items

query x {
  tokenEntityList(
    limit: 40,
    price_gt: 0,
    offset: 0,
    orderBy: [
    "price_ASC"
  ]) {
    id
    cheapest {
      price
    }
  }
}

let's say that we are on page 10 and we don't have more listed items , next page 11 , now we can use this query

query x {
  tokenEntityList(
    limit: 40,
    price_lte: 0,
    offset: 400,
    orderBy: [
    "price_ASC"
  ]) {
    id
    cheapest {
      price
    }
  }
}

  offset: (page - 1) * first.value,

page 11 would be 400 offset ((11 -1) * 40) , but that wouldn't be right since the first. 400 items of this query are not the same as the other one.

so we would need to change the offset logic to start from 0 here , also keep track of this new "page" for the second query

also some parts useListInfiniteScroll wouldn't work as expected , for exampletotal would be wrong

    const { entities, count } = getQueryResults(result.value)
    total.value = count

CleanShot 2024-01-22 at 15 17 20@2x

to make this work, wouldn't we end up adding more complexity?

I hope my concern is clearer now.

let me know if it's a non-issue and I'm missing something.

thanks

@vikiival
Copy link
Member

it's a data issue and should be fixed on backend

Like what specifically? What have you illustrated is matter of query

I hope my concern is clearer now.

I do not understand, sorry

Can you plase say

What is the expected output?

If it is this one

when this filter is applied it should show listed first - sorted by price and then the rest - by @exezbcz

It does not make sense show unlisted nft as they do not have price,
The only one thing that could make sense is to check how null behaves.

@vikiival vikiival changed the title tokenEntities orderBy price not working as expected orderBy price ascending should put prioritise "listed" tokens Jan 22, 2024
@exezbcz
Copy link
Member

exezbcz commented Jan 22, 2024

It does not make sense show unlisted nft as they do not have price,

yes it does, because you don't have to have buy now filter applied to see low to high price sort

@vikiival
Copy link
Member

vikiival commented Jan 23, 2024

Thanks for the explanation @exezbcz

therefore I have only two possible solutions:
Check how does the null behaves
or introduce new field like listed: boolean and hack it the way that sorting works

EDIT:

soo appartently nulls last would work

Just figure out how to write it in query

@vikiival
Copy link
Member

So got to the point:

  • we either do resolver for this
  • introduce something called "priority / score" that would give NFT score and therefore renders it with some score - unlisted will have lower score than listed

@yangwao
Copy link
Member

yangwao commented Jan 23, 2024

  • we either do resolver for this

isn't easier this one?

@vikiival
Copy link
Member

isn't easier this one?

Then you need to rewrite most of the view page. (I am not against)

@vikiival vikiival changed the title orderBy price ascending should put prioritise "listed" tokens TokenEntity resolver for explorer Jan 23, 2024
@vikiival
Copy link
Member

vikiival commented Jan 23, 2024

So apparently it exists on SubSquid.

Screenshot 2024-01-23 at 14 31 06

So guess #194 will fix it

@vikiival vikiival changed the title TokenEntity resolver for explorer orderBy price ascending should put prioritise "listed" tokens Jan 23, 2024
@vikiival vikiival linked a pull request Feb 13, 2024 that will close this issue
28 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants