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

Display WKN in MasterData #3498

Merged
merged 1 commit into from
Aug 14, 2023

Conversation

OnkelDok
Copy link
Member

@pfalcon
Copy link
Contributor

pfalcon commented Aug 12, 2023

Just a few words on devil's advocate side: local adhoc id's like WKN, CUSIP are known to be subsumed into ISIN. Even on the screenshot from the thread you link to, one can see: WKN: 110487, ISIN: DE0001104867. So, arguably not much new information is shown if adding WKN to the existing ISIN display (though WKN is not a perfect substring of ISIN :-( ). On the other hand, it takes a precious line at the top of that pane (line useless to anyone outside .de), requiring scrolling to see more info (at least now it's possible to scroll it after #3428).

On the other hand, I find it limiting that info pane shows taxonomies, a one kind of user-defined attribute for securities, and doesn't show other user-defined attributes. IMHO, what would be useful is to add a kind of "security card" to open on double-click with full-size chart, and as much as possible other info, optimized for viewing. (All/most info is available in security edit dialog, but that's optimized for editing.)

@OnkelDok
Copy link
Member Author

OnkelDok commented Aug 12, 2023

I just checked some securities: not every ISIN contains the WKN (MSFT, APC).
Maybe this applies only to german stocks (e.g. "Allianz, 840400)"?

Some months ago, @Nirus2000 made some good suggestion for some optimization of the security information pane by moving the MasterData from side panel into the chart title: #3004 (comment)
Maybe in the future this could be a good goal.

If you miss information in the pane, then we can discuss this. Please feel free to create a separate ticket for this with suggestions to have a better place for a discussion.
This PR is just a suggestion to add missing information with really low effort. Bigger changes should be discussed in separate ticket or in forum where more people can participate.

@Nirus2000
Copy link
Member

Nirus2000 commented Aug 12, 2023

Hello @OnkelDok
I see this just like you do. Your changes are good and meaningful for now. 👍🏻

Following up on my comment #3004 (comment), we could start by opening an ISSUE and adding it to our TODO list. In a brief phone call, I already spoke with @buchen about opening a project with the top 5 themes, where we can address projects like this one. I would place this under the top 3. I've mentioned the explanations for this before, but we should further discuss the content and milestones. Very good.
Could you please open an ISSUE for this, which we could then integrate into the project list? I think that's where the initial thoughts could develop. Additionally, we could schedule a small team meeting later to exchange thoughts again.

Thank you again... ;-)

@buchen
Copy link
Member

buchen commented Aug 14, 2023

Thanks for the implementation. Will merge.

I have created #3503 to discuss and track the evolution of the security details viewer.

@buchen buchen merged commit 0d62cdd into portfolio-performance:master Aug 14, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants