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

Feature/#574 chart legend #763

Open
wants to merge 32 commits into
base: develop
Choose a base branch
from
Open

Conversation

maria-vslvn
Copy link
Contributor

@maria-vslvn maria-vslvn commented Jul 6, 2021

Closes #574
Closes #723
Closes #796
chart legend tooltips styled, chart layout fixed

@github-actions
Copy link

github-actions bot commented Jul 7, 2021

  • 🔭 IDO UI: Easy IDO auction

@josojo
Copy link
Contributor

josojo commented Jul 7, 2021

Here are some points to improve:

In my browser it is shown like that:
image

I would have hoped to see legends more like this:
image

@elena-zh
Copy link

elena-zh commented Jul 7, 2021

Hi @maria-vslvn ,
in addition to Alex comment:

  1. tooltips are truncated in a mobile view
    image

  2. Price should contain a token's value as well. However, I have mentioned it in the current price logic implementation taks Display the current price in the legend of the chart - logic #769

  3. Current price seems to be very huge..
    image

#723 is fixed

Thanks

@maria-vslvn
Copy link
Contributor Author

fixed

@elena-zh
Copy link

elena-zh commented Jul 8, 2021

@maria-vslvn , the legend of the chart is still incorrect itself. It should look like this:
image
Each colored 'column' should have a name; 'current price' column should have a vertical dotted line in the center of it.
Thanks

@josojo
Copy link
Contributor

josojo commented Jul 11, 2021

Can we please write "supply" instead of suply?
Yes, would love to see the white dashed line implemented as in the proposal. Otherwise, it looks already gooood!

@elena-zh
Copy link

Hi @maria-vslvn , in addition to Alex comment.
Could we please do something in order not to overlap amcharts logo with our legend?
image
image

It is really difficult to click on the legend to call a tooltip and not to be navigated to the https://www.amcharts.com/ now.. Especially in a mobile device.

Thanks

@maria-vslvn
Copy link
Contributor Author

@elena-zh @josojo in order to change how the curr price is shown in the legend, i have to rewrite all the legend elements. i could spent some time on it? or maybe it's not so important. also i have a few questions about removing the charts copyright (https://stackoverflow.com/questions/38342247/how-to-remove-chart-copyright)

@elena-zh
Copy link

@maria-vslvn , another option, besides removing the charts copyright, might be shifting the legend to the right
image

@josojo
Copy link
Contributor

josojo commented Jul 14, 2021

also i have a few questions about removing the charts copyright

yes, I agree with Elena, we should find a different solution than removing thee copy right symbol

i could spent some time on it?

1-day work, it would be worth it for sure!

@maria-vslvn
Copy link
Contributor Author

@elena-zh i moved the legend to the top. looks weird a bit, so i could also create a custom container for the legend as a second variant and place it under the graph card and wrap it in the same border as the graph card or smt like that. the reason is the whole graph is an svg so it is hard to style it if the docs doesnt contain a prop or any info

@josojo
Copy link
Contributor

josojo commented Jul 20, 2021

Hm.... it's subjective. But I think that the top version is not looking better than the current version

@elena-zh
Copy link

@maria-vslvn , as far as I remember, we discussed an option to move the charts copyright to the top and leave the legend at the bottom.
Could we implement this?

Also, I do not see changes of the current price in the legend: it does not contains a vertical dotted line. It should look like this:
image

@maria-vslvn
Copy link
Contributor Author

@elena-zh i don't think we can move the copyright as there were no info in docs about it, so i decided to move the legend. maybe i have to find prettier solution..

@ramirotw
Copy link
Contributor

amcharts/amcharts4#2625

Hi,
You have to show the logo on all charts as is - that's the condition of the free license. Sorry.

@maria-vslvn
Copy link
Contributor Author

i just found a simple solutionScreenshot 2021-07-21 at 12.09.31.png

@josojo
Copy link
Contributor

josojo commented Jul 21, 2021

Can you show a screenshot together with the orderrplacement input field. I am afraid that changing the height will distort/stretch the input fields

@maria-vslvn
Copy link
Contributor Author

@josojo @elena-zh Screenshot 2021-07-21 at 16.47.12.png

@elena-zh
Copy link

@maria-vslvn
Place order section along with the chart area look good to me. the only odd issue is that I cannot open this PR in my iOS device in order to check how the chart looks there.

Still, changes of the current price in the legend are not yet implemented: it does not contains a vertical dotted line.
image

@elena-zh
Copy link

Hey @maria-vslvn , current price label LGTM!
However, chart tooltips went grey now.
Moreover, red tooltips are missing at all
grey
no red

Could you please fix it?

@maria-vslvn
Copy link
Contributor Author

@elena-zh please check

@elena-zh
Copy link

@maria-vslvn , Now all changes LGTM!
But there is a major issue: I cannot open this PR in iOS devices: I tried to open it in iPhone with 14.6 OS and in iPad mini with 14.7.1 iOS
image

So it would be great to fix it.

@maria-vslvn
Copy link
Contributor Author

maria-vslvn commented Sep 3, 2021

The link woks fine on android, my mac os chrome (not safari), iphone same. it throughs an error and i've googled something about safari issue in loookbehind support https://caniuse.com/js-regexp-lookbehind
so maybe anyone of you guys has any thoughts? @ramirotw @henrypalacios
Снимок экрана 2021-09-03 в 18 20 53

@elena-zh
Copy link

elena-zh commented Sep 9, 2021

The issue is still reproducible in iOS. Just note: the app is crashing only on an auction details page. Home, Docs, Auctions list pages can be opened.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants