-
Notifications
You must be signed in to change notification settings - Fork 31
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
163 remove tree data fix #231
163 remove tree data fix #231
Conversation
We can still remove name from tree_data but after discussion with @ElDeveloper, this will be postponed until further discussion with @antgonza. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just one comment from @antgonza and one question from me.
'sampVal': 1, | ||
'single_samp': False, | ||
'visible': True, | ||
'tree_data': {1: {'name': 'a', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kwcantrell would it be hard to replace the dictionary of dictionaries for a list of lists? It seems like the key for tree data is an index eitherway. In addition, if we can change this:
{'name': 'a', 'x2': 1, 'xc0': 2, 'xc1': 3, 'xr': 4, 'y2': 5, 'yc0': 6, 'yc1': 7, 'yr': 8}
To this:
['a', 1, 2, 3, 4, 5, 6, 7, 8]
This should save python from writing all the repeated key data, and save JavaScript from parsing all of that. We would just need to load the data in a container once it's parsed in JavaScript. If this is straightforward to do, then let's do it in this PR otherwise we can defer to a later PR since this goes beyond the original scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good suggestion @ElDeveloper. I'll see if I can change treeData to a list of lists. However, I will need to modify a fair amount of the empress class so we should wait to merge this PR until #230 is merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's defer this until later then.
@kwcantrell can you resolve the conflicts? Then it's ready to be merged. |
This reverts commit 5f879ef.
No description provided.