Chart Grid Layout #1092
Chart Grid Layout #1092
Conversation
| @@ -0,0 +1,582 @@ | |||
| <template> | |||
| <div class="d-flex flex-grow-1 chart-wrapper"> | |||
| <v-chart v-if="hasData" ref="candleChart" :theme="theme" autoresize manual-update /> | |||
There was a problem hiding this comment.
why would we need to duplicate the whole chart-component (tree) - instead of simply using what's there already?
That'll be a maintenance nightmare - so unless there's VERY good reasons to do so (it's unclear to me what the differences are - at a glance it seems to be "mostly" copy/pasted) - I'd prefer to keep one chart component only.
There was a problem hiding this comment.
why would we need to duplicate the whole chart-component (tree) - instead of simply using what's there already?
The code is roughly written to for the feature demonstration and feedback collection only.
There was a problem hiding this comment.
well but what's the difference to the original component (so what is "simpler" here??)
There was a problem hiding this comment.
The simplified view with removed labels, legends, and zooming possibilities.
The case chart gets more space - bringing more visibility to more minor elements on the charts - green/blue icons
There was a problem hiding this comment.
i understand labels / legends to some degree - but zooming should remain there (otherwise how would you imagine this to work on webserver mode - where you potentially have 10_000nds of candles? you'd not see anything anymore as you can't zoom.
I don't see this as separate components though (the change is not enough to justify a complete code duplication) - if we need this - then it can be properties on the existing component which disables these functionalities.
There was a problem hiding this comment.
if we need this - then it can be properties on the existing component which disable these functionalities.
Sure, that was an alternative option I preferred not to follow right from the beginning.
There was a problem hiding this comment.
It might be wrong to add more features to finished and stable software - totally understand that.
I was thinking I've spotted some gaps in UX, so I wanted to address them with this PR.
I'm ready to finalize the work if needed or close the PR which is also totally fine.
There was a problem hiding this comment.
That's perfectly fine 👍
Maybe also think about the "zoom" feature as optional - that can be enabled / disabled (For all sub-charts) globally? (so the user picks to either safe space - or to zoom).
i'm happy with the feature itself - but i also need to think about keeping the software itself sustainable / maintainable going forward - and a (90% duplicated) component or 2 will clearly not help that.
it's however also "too feature trimmed" - so it's no longer a useful chart (it's pretty small - and the moment you don't wanna look at overall trend - it's no longer useful).
it MAY work for your particular usecase - but for freqUI itself, i prefer the feature-complete version (though some things can be made optional).
| <template> | ||
| <div class="d-flex flex-column h-100"> | ||
| <div class="graphs-grid"> | ||
| <div v-for="pair in botStore.activeBot.whitelist" class="grid"> |
There was a problem hiding this comment.
I don't think that's a good idea (especially not in webserver mode - where there is no whitelist (the whitelist request will fail)- so this is basically pointless / nonfunctional in webserver mode).
instead, it should be a list (most likely based on the whitelist) - where users can select "i want to see this, this and that pair" (maybe a select all button - but ALL pairs shouldn't be the default).
Now what list we should use in webserver mode is a different topic ... one i'll need to look into further.
I can have a pairlist of 70 pairs - but showing me 70 charts will not allow me to compare anything, unless the 2 "comparing" pairs happen to be next to each other - which is pretty unlikely.
There was a problem hiding this comment.
I could be wrong but the usual chart is aways friendly, here:
In my working scenario I wan't to see how strategy behaves across all 100 pairs limited on a single chart on a screen, while an asset can only be changed from the dropdown - meaning I need to click 100 times.
There was a problem hiding this comment.
well i do understand this - that's the "select all" scenario (which i'd still limit to some sane number of pairs though to avoid memory problems).
by default, I'd however like to only show a selected few.
That'll also reduce the amount of data necessary - and keep the UI responsive.
There was a problem hiding this comment.
by default, I'd however like to only show a selected few.
Agree, the whole collection of pairs is to be displayed in a paginated manner and loaded dynamically. say 10 items per page.
|
@ivanproskuryakov any news on this? i think this would be a great feature to have - so it'd be a shame to end up as abandoned PR ... ;) |
|
@xmatthias Hi, I'll need some time to have it finished, thanks for the support. Been flooded with my primary job. |
See #1091