Skip to content
This repository was archived by the owner on Dec 5, 2025. It is now read-only.

Conversation

@rhaicode
Copy link
Contributor

@rhaicode rhaicode commented Nov 12, 2021

Thank you for your contribution to the KodaDot NFT gallery.

👇 Let's do a quick check before the merge.

PR type

  • Bugfix
  • Feature
  • Refactoring

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 respect high code quality standards
  • I've didn't break any original functionality
  • I've posted screenshot of demonstrated change in this PR

Optional

  • I've tested PR on mobile and everything works
  • I found edge cases

What's new?

Had issue bounty label?

  • Fill up your KSM address: Payout

Community participation

Screenshot

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

@netlify
Copy link

netlify bot commented Nov 12, 2021

✔️ Deploy Preview for koda-nuxt ready!

🔨 Explore the source changes: 9459fd3

🔍 Inspect the deploy log: https://app.netlify.com/sites/koda-nuxt/deploys/61a0fd6f120b9b0008d39838

😎 Browse the preview: https://deploy-preview-1110--koda-nuxt.netlify.app

@rhaicode
Copy link
Contributor Author

rhaicode commented Nov 12, 2021

Graph found at: /en/rmrk/gallery/:id

Before (ECharts):
before-echarts

After (ChartJS with chartjs-adapter-luxon) (hovered state):
after-using-chartjs

There are some follow-up questions I want to ask:
1.) Are we going to follow the 24-hour format for the x-axis?
2.) Are we going to support data zooming and panning?

Thanks a lot!

@rhaicode rhaicode mentioned this pull request Nov 12, 2021
@yangwao
Copy link
Member

yangwao commented Nov 12, 2021

Amazing progress!

1.) Are we going to follow the 24-hour format for the x-axis?

Thinking we should with our ethos, least possible information, that means having.
Let's keep 24 hours, just HH if possible and have precise HH: MM at the event point. Will see how it looks like.

2 ) Are we going to support data zooming and panning?

That's a tricky one, how hard it would be and how easy would be interacting with that on mobile?

I would propose

  • item detail -- w/o zoom/pan - I'm thinking to add pre-made time frames. 7d/14d/1m where 7d could be the default
  • collection history -- w/ zoom/pan -- as there is a lot of data

@rhaicode
Copy link
Contributor Author

1.) All right, got it. I'll be working on it right away.
2.) Unlike ECharts that already supports this by providing a dataZoom property, ChartJS doesn't support it by default. We could add a plugin like chartjs-plugin-zoom

@yangwao
Copy link
Member

yangwao commented Nov 12, 2021

2.) Unlike ECharts that already supports this by providing a dataZoom property, ChartJS doesn't support it by default. We could add a plugin like chartjs-plugin-zoom

Works well on mobile, let's try using it!

@yangwao
Copy link
Member

yangwao commented Nov 12, 2021

Chatted with @rhaicode and will migrate collection chart to main-nuxt as well

@rhaicode
Copy link
Contributor Author

rhaicode commented Nov 13, 2021

Hello @yangwao. I'd like to ask for your feedback with the chart having zoom plugin?

https://screencast-o-matic.com/watch/crXIezVI11T

@rhaicode
Copy link
Contributor Author

rhaicode commented Nov 13, 2021

I would propose

item detail -- w/o zoom/pan - I'm thinking to add pre-made time frames. 7d/14d/1m where 7d could be the default

Currently working on this. I'll remove the zoom/pan from the previous zoom integ and just provide pre-made time frames.

collection history -- w/ zoom/pan -- as there is a lot of data

In order to work on the graph under the Activity tab which is under the Collections tab, the Collections functionality should be added as well since main-nuxt doesn't have the Collections tab above.

@yangwao
Copy link
Member

yangwao commented Nov 13, 2021

Activity tab which is under the Collections tab, the Collections functionality should be added as well since main-nuxt doesn't have the Collections tab above.

Yes, let's add it :)

@yangwao
Copy link
Member

yangwao commented Nov 13, 2021

Hello @yangwao. I'd like to ask for your feedback with the chart having zoom plugin?

https://screencast-o-matic.com/watch/crXIezVI11T

Is there possibility to fix Y axis not go below 0 ? as price will be always >= 0.
Otherwise good work there, really looking forward!

@rhaicode rhaicode marked this pull request as draft November 13, 2021 08:24
@yangwao
Copy link
Member

yangwao commented Nov 15, 2021

Stradford pinged me on Thursday will check on this one

@rhaicode rhaicode marked this pull request as ready for review November 17, 2021 09:13
@rhaicode
Copy link
Contributor Author

rhaicode commented Nov 17, 2021

Hello, @yangwao ! This is the screen recording link for the possible v1 for the chartjs integration.
http://somup.com/crXbQl0QE8

@yangwao yangwao requested a review from roiLeo November 17, 2021 12:05
@yangwao
Copy link
Member

yangwao commented Nov 17, 2021

For the collection view, let's make it 100% width.

image

For item detail, let's add colours.
Green for BUY, List can be as it is.

image

I sense that zoom in is proportional, but zoom out is not symmetric?

Screen.Recording.2021-11-17.at.13.27.44.mov

Otherwise LGTM for v1 ! :)

@yangwao yangwao mentioned this pull request Nov 17, 2021
@rhaicode
Copy link
Contributor Author

rhaicode commented Nov 17, 2021

For the collection view, let's make it 100% width.

  • Got this.

For item detail, let's add colours.
Green for BUY, List can be as it is.

  • Does this mean the color of the dots on the price chart?

I sense that zoom in is proportional, but zoom out is not symmetric?

  • I have only set the minimum range for zoom out to be 0 KSM (so that we won't be able to see negative KSM values). However, what do you mean by proportional and symmetric within this context? I'm only using the default zooming of chartjs-plugin-zoom.

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.

Hey!
I would love to see what it looks like but I can't run nuxt branch right now.
Why do we have 2 different Charts component? Different feature/data?

@rhaicode
Copy link
Contributor Author

rhaicode commented Nov 17, 2021

@roiLeo hello! yes, we have two since i just pulled from main-nuxt and removed ECharts there. I'll be working on the requested code changes.

@yangwao
Copy link
Member

yangwao commented Nov 17, 2021

For item detail, let's add colours.
Green for BUY, List can be as it is.

  • Does this mean the color of the dots on the price chart?

Let's keep it the same as a graph for collection activity with lines :)

Why do we have 2 different Charts component? Different feature/data?

@roiLeo we will discard the old chart engine for this one which should be more native and works much better on mobile.

@yangwao
Copy link
Member

yangwao commented Nov 21, 2021

but then it seems that the endpoint for the charts at collections>activity tab is not working? i'm only receiving no data in my end

I guess needs to have more data there, cc @dezine2dev

@yangwao
Copy link
Member

yangwao commented Nov 21, 2021

Latest chart for /en/rmrk/gallery/:id http://somup.com/crXrlU0oz5

How does it look like on NFT which has more interactions?

@rhaicode
Copy link
Contributor Author

@yangwao Something like this? http://somup.com/crX3n80DQE

@yangwao
Copy link
Member

yangwao commented Nov 22, 2021

@yangwao Something like this? http://somup.com/crX3n80DQE

okay, doable I guess!

I guess we are waiting for the graph for collections and it's everything done?

Check recent merge #1212 and refactor for charts was made in main, in best case migrate that code and I'm happy to top your bounty for this issue!

@yangwao
Copy link
Member

yangwao commented Nov 22, 2021

@rhaicode
Copy link
Contributor Author

@yangwao The code for both charts come from main too then just migrated it here to main-nuxt. @roiLeo could also verify

@yangwao
Copy link
Member

yangwao commented Nov 22, 2021

Hey! I would love to see what it looks like but I can't run nuxt branch right now.

@roiLeo https://deploy-preview-1110--koda-nuxt.netlify.app/en/rmrk/collection/4a5668af20c28c074f-KSMDUCKZ

@yangwao yangwao requested a review from vikiival November 22, 2021 17:28
@yangwao
Copy link
Member

yangwao commented Nov 23, 2021

Hey! I would love to see what it looks like but I can't run nuxt branch right now.

@roiLeo https://deploy-preview-1110--koda-nuxt.netlify.app/en/rmrk/collection/4a5668af20c28c074f-KSMDUCKZ

@roiLeo let's merge it then if it's good!

@yangwao
Copy link
Member

yangwao commented Nov 23, 2021

payout $400 sent meanwhile! https://kusama.subscan.io/extrinsic/0x5945e757597ec9c581749d5bad66c52ba3b3148fef35ca60a3801e1f98c3f093

@rhaicode
Copy link
Contributor Author

received! thanks a lot 🚀

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.

Collection charts LGTM, I'm not able to test GalleryItem chart as route not working for now.
We could increase chart height on mobile view (167px is kinda small to me)

@rhaicode
Copy link
Contributor Author

rhaicode commented Nov 23, 2021

I'll provide follow up commits. 👍

Copy link
Member

@vikiival vikiival left a comment

Choose a reason for hiding this comment

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

Looks legit.
also use linter

@rhaicode
Copy link
Contributor Author

@roiLeo @vikiival this one's ready for review

@vikiival
Copy link
Member

Deff check tomorrow !

@vikiival
Copy link
Member

vikiival commented Nov 26, 2021

Build is failing because of #1273

@vikiival vikiival mentioned this pull request Nov 26, 2021
10 tasks
@rhaicode
Copy link
Contributor Author

@vikiival implemented requested code changes

@yangwao
Copy link
Member

yangwao commented Nov 26, 2021

Amazing job @vikiival @roiLeo @rhaicode
looking forward this stuff live soon(tm) ! :)

@yangwao yangwao merged commit 137cf96 into kodadot:main-nuxt Nov 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

New charts engine

4 participants