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

feat: Add volume sort to ExploreSort #5699

Conversation

Matehoo
Copy link
Contributor

@Matehoo Matehoo commented Apr 17, 2023

Thank you for your contribution to the KodaDot NFT gallery.

👇 _ Let's make a quick check before the contribution.

PR Type

  • Bugfix
  • Feature
  • Refactoring

Context

Before submitting pull request, please make sure:

  • My contribution builds clean without any errors or warnings
  • I've merged recent default branch -- main and I've no conflicts
  • I've tried to respect high code quality standards
  • I've didn't break any original functionality
  • I've posted a screenshot of demonstrated change in this PR

Optional

  • I've tested it at </bsx/collection>
  • I've tested PR on mobile
  • I've written unit tests 🧪
  • I've found edge cases

Had issue bounty label?

  • Fill up your KSM address: Payout

Community participation

Screenshot 📸

  • My fix has changed something on UI; a screenshot is best to understand changes for others.

Screenshot 2023-04-17 at 12 45 48

@Matehoo Matehoo requested a review from a team as a code owner April 17, 2023 10:45
@Matehoo Matehoo requested review from preschian and daiagi and removed request for a team April 17, 2023 10:45
@kodabot
Copy link
Collaborator

kodabot commented Apr 17, 2023

SUCCESS @Matehoo PR for issue #5678 which is assigned to you. Please wait for review and don't hesitate to grab another issue in the meantime!

@netlify
Copy link

netlify bot commented Apr 17, 2023

Deploy Preview for koda-canary ready!

Name Link
🔨 Latest commit 4eb48a1
🔍 Latest deploy log https://app.netlify.com/sites/koda-canary/deploys/64ddee8f43022900081ce70b
😎 Deploy Preview https://deploy-preview-5699--koda-canary.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@reviewpad
Copy link
Contributor

reviewpad bot commented Apr 17, 2023

AI-Generated Summary: This pull request adds a new volume sort feature to the ExploreSort menu. The changes include adding two new sorting options, "Most Volume" and "Least Volume," to both the locales/en.json and utils/constants.ts files.

@reviewpad reviewpad bot added small Pull request is small waiting-for-review labels Apr 17, 2023
Copy link
Contributor

@roiLeo roiLeo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any sorting by volume option in collectionEntities

@roiLeo roiLeo added the S-changes-requested-🤞 PR is almost good to go, just some fine tunning label Apr 17, 2023
@Matehoo
Copy link
Contributor Author

Matehoo commented Apr 17, 2023

Volume sorting is supported in latest unreleased indexer, no ETA on the release (but should be soon)

@roiLeo
Copy link
Contributor

roiLeo commented Apr 17, 2023

Volume sorting is supported in latest unreleased indexer, no ETA on the release (but should be soon)

This is changed only on rubick indexer, we need to change CollectionEntity Schema in kodadot/snek repo to make it work on bsx chain

@Matehoo
Copy link
Contributor Author

Matehoo commented Apr 17, 2023

This is changed only on rubick indexer, we need to change CollectionEntity Schema in kodadot/snek repo to make it work on bsx chain

Oh shoot, you are right, that change was not merged yet (kodadot/snek#170).
When it is merged ILYK

@yangwao yangwao marked this pull request as draft April 20, 2023 11:00
@yangwao yangwao mentioned this pull request Apr 22, 2023
@yangwao
Copy link
Member

yangwao commented May 1, 2023

Volume sorting is supported in latest unreleased indexer, no ETA on the release (but should be soon)

"id": 4638,
"name": "v3",
"squidName": "marck",
"status": "SYNCED",
"syncStatus": {
"totalBlocks": 17722593,
"currentBlock": 17722593

marckV3 seems synced, is it stable already @vikiival ?

@vikiival
Copy link
Member

vikiival commented May 3, 2023

Synced and using on prod

@yangwao
Copy link
Member

yangwao commented May 19, 2023

@Matehoo this can go out I guess?

@Matehoo
Copy link
Contributor Author

Matehoo commented May 19, 2023

@Matehoo this can go out I guess?

@vikiival are all indexers with this feature in prod?

@vikiival
Copy link
Member

vikiival commented May 19, 2023

We can still conditionally render that for snek and marck

const { urlPrefix } = usePrefix()
['bsx', 'ksm'].includes(urlPrefix.value)

@roiLeo
Copy link
Contributor

roiLeo commented May 23, 2023

please release 004 as this won't work on snek

Copy link
Contributor

@roiLeo roiLeo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

snek https://squid.subsquid.io/sneck/graphql has been released in #6125

@roiLeo roiLeo marked this pull request as ready for review May 30, 2023 08:28
@roiLeo roiLeo removed the S-changes-requested-🤞 PR is almost good to go, just some fine tunning label May 30, 2023
@Matehoo
Copy link
Contributor Author

Matehoo commented Jun 5, 2023

@preschian looks like sneck is back again?

@vikiival Do you have ETA for rubick release?

@yangwao
Copy link
Member

yangwao commented Aug 10, 2023

@vikiival
Copy link
Member

vikiival commented Aug 11, 2023

Made low budget release of rubick

should be up soon

Supplemental as I have false positives

@vikiival
Copy link
Member

Screenshot 2023-08-11 at 12 58 36

Making new release yolo

@roiLeo
Copy link
Contributor

roiLeo commented Aug 11, 2023

Making new release yolo

Please don't forget snekk 004

@vikiival
Copy link
Member

oof so short adventure

@vikiival
Copy link
Member

Making new release yolo

Please don't forget snekk 004

I already made deploy for it

https://app.subsquid.io/squids/sneck/v1

CREATED: 28.05.2023

@roiLeo
Copy link
Contributor

roiLeo commented Aug 11, 2023

Making new release yolo

Please don't forget snekk 004

I already made deploy for it

https://app.subsquid.io/squids/sneck/v1

CREATED: 28.05.2023

Oooh didn't knew about that one, then we need to update snek endpoint (we still query 004)

@roiLeo roiLeo mentioned this pull request Aug 11, 2023
@vikiival
Copy link
Member

Screenshot 2023-08-11 at 16 12 39

@vikiival
Copy link
Member

Another issue with same code :)))))))

Screenshot 2023-08-14 at 16 10 50

@yangwao
Copy link
Member

yangwao commented Aug 16, 2023

any updates?

@codeclimate
Copy link

codeclimate bot commented Aug 17, 2023

Code Climate has analyzed commit 4eb48a1 and detected 0 issues on this pull request.

View more on Code Climate.

@vikiival
Copy link
Member

any updates?

rubick updated in 4eb48a1

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
2.2% 2.2% Duplication

@yangwao
Copy link
Member

yangwao commented Aug 17, 2023

@prury can you check this one 😌 and then let's merge it

@prury
Copy link
Member

prury commented Aug 17, 2023

RMRK 1 and 2, as soon as i sort by volume:
image

@vikiival
Copy link
Member

Oki thanks will recheck

@yangwao
Copy link
Member

yangwao commented Aug 23, 2023

Can we have it merged?

@roiLeo
Copy link
Contributor

roiLeo commented Aug 23, 2023

Can we have it merged?

Not yet

@vikiival
Copy link
Member

Can we have it merged?

Do you remember what was the issue with marck v3? 🥺

It is needed for this.
And for some reason we reverted that

@yangwao
Copy link
Member

yangwao commented Aug 26, 2023

Do you remember what was the issue with marck v3? 🥺

no, link to issue?

It is needed for this.

maybe do some hotfix or exclude volume sort for that for now.

@vikiival
Copy link
Member

no, link to issue?

#5853

Most probably this was the reason

@yangwao
Copy link
Member

yangwao commented Aug 29, 2023

Most probably this was the reason

But that's 4 mo old?

@vikiival
Copy link
Member

Won't make it for now

@vikiival vikiival closed this Sep 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-changes-requested-🤞 PR is almost good to go, just some fine tunning small Pull request is small waiting-for-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sort collections by volume
8 participants