Add info relative to clicked node values to nodeInfoDisplayer #3
Conversation
|
Hi @David-Desmaisons, do you accept PRs? I have a couple more ideas but I will not even bother creating the PRs if I know they don't stand a chance. |
|
Hello @isaacbernat , I accept PRs!
For further changes, the best way should to create first an issue so we can discuss the pertinence of the changes for this repo and once we agreed on the feature, you could go ahead and create a PR. |
|
Hi @David-Desmaisons , sorry if I sounded too harsh/impatient. I just saw you updated several commits to this repo and that you did not reply to the PR, so I thought you were actively ignoring it. I also want to take the chance to praise the excellent work done on this component. |
| <span>{{percentage}}</span><br/> {{description}} | ||
| <span>{{percentage}}</span> | ||
| <br/> {{description}} | ||
| <br/> ({{current.value}} / {{base}}) |
There was a problem hiding this comment.
I would remove this:
<br/> ({{current.value}} / {{base}})
There was a problem hiding this comment.
I think having the absolute values (not only %) can be useful. I admit it may also feel a bit too much information. Maybe a solution would be a parameter to decide wether to show absolute values or not?
Another way of displaying the information would be some kind of tooltip on the mouse pointer when hovering over a section.
| percentage() { | ||
| if (this.current == null || this.root == null) { | ||
| return null; | ||
| } | ||
|
|
||
| const percentage = (100 * this.current.value) / this.root.value; | ||
| const percentage = (100 * this.current.value) / this.base; |
There was a problem hiding this comment.
This new way of calculating porcentage is interesting. The only drwaback is that it could be more than 100% when zooming. I will take here the min beetween 100 and the actual value.
There was a problem hiding this comment.
👍A value bigger than 100% might look confusing.
|
Hi @David-Desmaisons, was that feedback sufficient? |
David-Desmaisons
changed the base branch from
master
to
improve-node-diplayer
|
Merged and available in version 0.20.0 |
Summary: The goal of this PR is to add more relevant information to the end user with as little burden as possible.
App.vue
descriptionvalue is shortened.nodeInfoDisplayer
breadcrumbTrail
Here is a screenshot of how the changes would look in the example:
PS: I have never built anything using Vue.js and it's been a few years since I did anything serious at all in js. Feel free to make the changes you deem necessary.