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

remove nomics and replace chart #2239

Merged
merged 9 commits into from
Apr 7, 2023
Merged

remove nomics and replace chart #2239

merged 9 commits into from
Apr 7, 2023

Conversation

smk762
Copy link
Collaborator

@smk762 smk762 commented Apr 4, 2023

Closes #2238

To test:

  • check dex pro view chart. resize / maximise window. change pair. there should be no flicker or overflow.

We dont have i or this widget yet, though it uses tickers by default so it the ticker is supported a chart will display.
This has some known issues:

  • The ticker may link to the wrong coin
    image
  • the ticker may not be supported
    image

Until we can compile a list of correct/working tickers, I'll add an exclusion list for coins known to be no good. Please list any you encounter in this PR.

@smk762 smk762 marked this pull request as draft April 4, 2023 17:54
@cipig
Copy link
Member

cipig commented Apr 4, 2023

we could add a new file to https://github.com/KomodoPlatform/coins/tree/master/api_ids for livecoinwatch

can we also please use 7d charts, instead of 1d? just set lcw-period="w"

@smk762
Copy link
Collaborator Author

smk762 commented Apr 5, 2023

can we also please use 7d charts, instead of 1d? just set lcw-period="w"

Done. I've not implemented an exclusion list yet, but will populate it from review feedback.

@smk762 smk762 marked this pull request as ready for review April 5, 2023 08:25
@cipig
Copy link
Member

cipig commented Apr 5, 2023

looks good, thanks
we need to remove the chain from the ticker though, eg -BEP20 and also -segwit because:
image
image

cipig
cipig previously approved these changes Apr 5, 2023
Copy link
Member

@cipig cipig left a comment

Choose a reason for hiding this comment

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

works fine now, thanks
this error shows up when i switch pairs, but looks like it has no negative effect, and idk if it's even related
[12:42:53] [error] [main.prerequisites.hpp:96] [304992]: Error: <svg> attribute viewBox: Unexpected end of attribute. Expected number, "".

Copy link
Contributor

@Canialon Canialon left a comment

Choose a reason for hiding this comment

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

  • looks like it can be a scrollbar if it is no chart, i guess it is not very good, will be nice to not cause it.

image

  • in pair of fiat and not fiat coin: with some test coins, it is no data but with (screenshot 1) but with some test coins the chart displays with $ (screenshot 2)

image

image

-sometimes size broken
image

  • the resizing looks weird and slow, also can just freeze with "loading data"
Screen.Recording.2023-04-05.at.15.02.13.mov
Screen.Recording.2023-04-05.at.15.06.33.mov

UPD: tested on MacOS

@cipig
Copy link
Member

cipig commented Apr 5, 2023

strange, i don't have those problems with the sidebar, the chart is not reloaded... the only thing is that everything is moved to the right, even if it doesn't fit into the window, like this:
image

btw, do we need this sidebar animation thingy? i guess it would make some things easier if we would simply disable the animation/resize of the sidebar

when i resize, the loading is indeed somewhat slow, but i don't care, i don't resize all the time
btw, saving the window size would be nice, then i wouldn't have to resize at all, see #2207

EDIT: the content is moved to the right instead of resized when i use the min width... if i make the windows bigger first and then hover the side bar, the part with the chart is resized and then it reloads here too

Copy link

@endrilickollari endrilickollari left a comment

Choose a reason for hiding this comment

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

Sidebar loading happening in Windows 10 also, sometimes it loads faster sometimes it gets stuck also during resizing it does the same thing.

Video:

AtomicDex.Desktop.test.chart.in.sidebar.mp4

Also when trying to trade test coins message appears:

image

OS: Windows 10
Resolution: 1920 x 1080

@smk762
Copy link
Collaborator Author

smk762 commented Apr 6, 2023

works fine now, thanks this error shows up when i switch pairs, but looks like it has no negative effect, and idk if it's even related [12:42:53] [error] [main.prerequisites.hpp:96] [304992]: Error: <svg> attribute viewBox: Unexpected end of attribute. Expected number, "".

This error is thrown from the widget's own script. It also comes up in the console on https://www.livecoinwatch.com/widgets

@smk762
Copy link
Collaborator Author

smk762 commented Apr 6, 2023

-sometimes size broken

  • the resizing looks weird and slow, also can just freeze with "loading data"

Sidebar loading happening in Windows 10 also, sometimes it loads faster sometimes it gets stuck also during resizing it does the same thing.

I've now set it to use a fixed width, which should resolve the resizing issues.
Testcoins should also now be filtered out (if either ticker is a testcoin, there will be no chart)

@smk762
Copy link
Collaborator Author

smk762 commented Apr 6, 2023

Also when trying to trade test coins message appears:

This is expected, though I might tweak the wording, because yet implies there will be a chart in future (but for test coins this is unlikely)

@smk762
Copy link
Collaborator Author

smk762 commented Apr 6, 2023

Ready for re-review. You will have to reset assets config for the latest changes in coins repo (KomodoPlatform/coins#689) to apply

chart-update.mp4

@smk762
Copy link
Collaborator Author

smk762 commented Apr 6, 2023

It seems for some minor coins which are supported by livecoinwatch may not be supported as a pair. I can't think of a method to filter these out without using a pair exclusion list.

known-issue-1.mp4

As it is only affecting less common pairs, I'll make this an issue for future (so not a blocker for this PR)

cipig
cipig previously approved these changes Apr 6, 2023
Copy link

@endrilickollari endrilickollari left a comment

Choose a reason for hiding this comment

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

Chart looks very good now

video:

AtomicDex.Desktop.2023-04-06.19-56-39.mp4

OS: Windows 10

Copy link
Contributor

@Canialon Canialon left a comment

Choose a reason for hiding this comment

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

looks good now:D

@smk762 smk762 merged commit 96b135f into dev Apr 7, 2023
@smk762 smk762 deleted the rm_nomics branch August 7, 2023 07:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FR]: Remove nomics charts
4 participants