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

Chart Grid Layout #1092

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

ivanproskuryakov
Copy link

See #1091

@@ -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 />
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

@ivanproskuryakov ivanproskuryakov Jan 11, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

well but what's the difference to the original component (so what is "simpler" here??)

Copy link
Author

@ivanproskuryakov ivanproskuryakov Jan 11, 2023

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

@ivanproskuryakov ivanproskuryakov Jan 11, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@xmatthias xmatthias Jan 11, 2023

Choose a reason for hiding this comment

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

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">
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

@ivanproskuryakov ivanproskuryakov Jan 11, 2023

Choose a reason for hiding this comment

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

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.

Screenshot 2023-01-11 at 17 49 06

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

@xmatthias xmatthias linked an issue Jan 11, 2023 that may be closed by this pull request
@xmatthias xmatthias changed the title Grid Layout #1091 Chart Grid Layout Jan 11, 2023
@xmatthias
Copy link
Member

@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 ... ;)

@ivanproskuryakov
Copy link
Author

@xmatthias Hi, I'll need some time to have it finished, thanks for the support. Been flooded with my primary job.

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.

Chart Grid Layout
2 participants