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

Add derived data series to statements of assets chart #4403

Merged
merged 1 commit into from
Dec 21, 2024

Conversation

buchen
Copy link
Member

@buchen buchen commented Dec 20, 2024

Uses DerivedDataSeries to hold a reference to the underlying data series (base) and the aspect to be shown (ClientDataSeries).

Issue: #3754
Issue: #4235
Issue: #4396

@buchen
Copy link
Member Author

buchen commented Dec 20, 2024

@mierin12, as commented in the other PR:

Based your code, I did some more refactored:

  • First, I renamed the objects and types to be "derived" and "aspect" (similar to the widget on the dashboard)
  • Second, the DerivedDataSeries is now an object that holds the reference to the underlying data series (base) and the aspect to be used. This way, we can re-used the (potentially) already cached data series.
  • Third, I now create the data series based on the already created underlying data series.

If you have time, I would appreciate if you can have a look

@buchen buchen force-pushed the feature_derived_data_series_in_assets_chart branch from 1ff25ff to 9a59081 Compare December 20, 2024 09:49
@mierin12
Copy link
Contributor

mierin12 commented Dec 20, 2024

Hello @buchen , thanks for checking it !!
I have one comment related to your third point. Building from the availableSeries simplifies the code a lot and works very well, except for the isLine and isArea value. Those parameters are set from the baseDataSeries but should actually depend here on the Aspect itself, not the baseDataSeries. Ex : Interest should always be Bar type : "Interest : Taxonomies A" should be a Bar even if by default Taxonomy A will be a Line.

Proposition to fix it : something this way should work : a7cda52

So far my only comment. I will continue to double check.

I think the naming "Aspect : data series" is definitely an improvement for the Legend and Tooltip over "Aspect (data series)". But I am wondering if it is too much to repeat the Aspect in the subfolders of selection dialog.
Capture d'écran 2024-03-01 034901
In particular for Taxonomies that already have a long name.

vs
Capture d’écran 2024-12-20 210121
that I had in mind.
But as it is purely a UI question, as you prefer.
But it is true that it is very clear this way, in particular in case of long list of portfolio, sometimes you do not see anymore the top level Aspect folder, with my tree you do not know anymore where you are, while in your tree it is always clear.

new DerivedDataSeries(baseDataSeries, type), label, wheel.next());

dataSeries.setLineChart(baseDataSeries.isLineChart());
dataSeries.setShowArea(baseDataSeries.isShowArea());

This comment was marked as resolved.

@mierin12
Copy link
Contributor

I do not know how to make code suggestion in the review but my proposition of update is an in this :
a7cda52

@mierin12
Copy link
Contributor

No other comment than the one about the Line/Area settings, I think this works great !! Thanks a lot for looking into it, I think it is a super feature !

(for another feature in the future I am wondering if a similar approach could be used in the Performance Chart to use daily/weekly.. % for any series :) !!)

@buchen
Copy link
Member Author

buchen commented Dec 21, 2024

Thanks @mierin12 for reviewing!

Both comments make sense. I missed the showArea/lineChart. The labels should be fixed as well.

(for another feature in the future I am wondering if a similar approach could be used in the Performance Chart to use daily/weekly.. % for any series :) !!)

Yes!! Agree.

@buchen buchen force-pushed the feature_derived_data_series_in_assets_chart branch from 0b46f52 to 085797f Compare December 21, 2024 08:14
@buchen
Copy link
Member Author

buchen commented Dec 21, 2024

The dialog should not be uncluttered (sorry for the German screenshot, but you get the idea).

Bildschirmfoto 2024-12-21 um 09 09 58

@buchen
Copy link
Member Author

buchen commented Dec 21, 2024

Uses DerivedDataSeries to hold a reference to the underlying data
series (base) and the aspect to be shown (ClientDataSeries).

Issue: #3754
Issue: #4235
Issue: #4396
Co-authored-by: Shivam Paw <[email protected]>
Co-authored-by: mierin12 <[email protected]>
Co-authored-by: Andreas Buchen <[email protected]>
@buchen buchen force-pushed the feature_derived_data_series_in_assets_chart branch from 085797f to 41cf1d3 Compare December 21, 2024 08:18
@mierin12
Copy link
Contributor

Perfect ! I have looked again, I do not see any issue, it looks good !

@buchen buchen merged commit 28b7532 into master Dec 21, 2024
2 checks passed
@buchen buchen deleted the feature_derived_data_series_in_assets_chart branch December 30, 2024 10:03
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.

3 participants