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

Global tree properties #212

Merged
merged 9 commits into from
Jun 29, 2020
Merged

Conversation

kwcantrell
Copy link
Collaborator

@kwcantrell kwcantrell commented Jun 25, 2020

This PR resolves #172 and #167. In addition, Empress now starts with the root node at center of screen and when user searches for a node, that node is place at the center of screen. Finally, the last point of #169 is address,namely, the green node goes away when the node menu disappears.

I wasn't sure how to best add the GUI elements so if you have suggestions on how to better implement them (mainly the center tree button) let me know.

@kwcantrell kwcantrell linked an issue Jun 25, 2020 that may be closed by this pull request
@fedarko fedarko self-requested a review June 25, 2020 22:32
Copy link
Collaborator

@fedarko fedarko left a comment

Choose a reason for hiding this comment

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

This looks great @kwcantrell! A few small suggestions, but having these features in will be super useful.

empress/support_files/js/drawer.js Outdated Show resolved Hide resolved
empress/support_files/js/drawer.js Outdated Show resolved Hide resolved
empress/support_files/js/empress.js Outdated Show resolved Hide resolved
empress/support_files/js/select-node-menu.js Show resolved Hide resolved
empress/support_files/templates/side-panel.html Outdated Show resolved Hide resolved
Make the root of the tree appear at the center of the screen.
</p>
<p>
<button id="center-tree-btn">Center Tree</button>
Copy link
Collaborator

Choose a reason for hiding this comment

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

The GUI is ok as is -- I think things are mostly self-explanatory -- but for my two cents, I'd suggest switching around the order of these two controls and their descriptions (putting the controls before their descriptions) and then applying the indented class to the description <p>s. Here's what this looks like on my end:

image

This 1) makes it clearer which descriptions map to which controls, and 2) makes these descriptions visually consistent with the "Only use tip-level metadata?" checkbox in the feature metadata section.

In general, I think this is the sort of situation where tooltips (#138) would be really useful (probably for later on, though... IMO there are more important things for us to address before taking the time to add those). Having tooltips set up would let us do things like just show a "Center Tree" button by itself, with a (?) or something next to it which would pop up this help text if people have further questions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, I'll adjust the order/description to be consistent with the others.

empress/support_files/js/drawer.js Outdated Show resolved Hide resolved
empress/support_files/js/drawer.js Show resolved Hide resolved
empress/support_files/js/drawer.js Show resolved Hide resolved
Copy link
Member

@ElDeveloper ElDeveloper left a comment

Choose a reason for hiding this comment

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

Looks great, just two minor suggestions, and also I think we should call re-center after the layout is switched.

Would it be possible to center the camera not at the root but instead in the mid point of the x and y ranges for each layout? Maybe worth leaving it for a different issue if that's too complicated.

Make the root of the tree appear at the center of the screen.
</p>
<p>
<button id="center-tree-btn">Center Tree</button>
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to be clearer:

Suggested change
<button id="center-tree-btn">Center Tree</button>
<button id="center-tree-btn">Reset Camera</button>

@@ -9,6 +9,25 @@
</p>
</div>

<!-- Global Tree properties -->
<button class="side-header collapsible">Tree Properties</button>
Copy link
Member

Choose a reason for hiding this comment

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

Can you move Tree Properties to the bottom of the side panel? i.e. after animations. Motivation is to keep the most frequently used things at the top (which I think might be a tie between sample and feature metadata).

Copy link
Member

@ElDeveloper ElDeveloper left a comment

Choose a reason for hiding this comment

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

Thanks so much @kwcantrell!

* Centers the viewing window at the average of the current layout.
*/
Empress.prototype.centerLayoutAvgPoint = function() {
if (!(this._currentLayout in this.layoutAvgPoint)) {
Copy link
Member

Choose a reason for hiding this comment

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

Nice, this helps defer the work until it is actually needed and it caches the results. Very useful!

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.

dude where's my tree? (add recenter camera) Support toggling node circles on/off?
3 participants