Fix clicking on nodes not in table; refactor and test node selection menu; fix "Add" button #432
Conversation
still gotta clean up tests, and actually integrate this into the selection menu, and etc. but a start.
Closes biocore#314. However, the UI here could still be improved a lot! So gonna tidy things up a bit before getting this PR in. May knock out biocore#272 while we're at it.
Still kinda broken, but I think i have an idea of how to fix it
Because manually testing this is going to give me a stroke
Didn't seem like there was much of a reason to keep initialize out of the selected node menu constructor, so just went ahead and moved it there.
since the tip case is afaict literally impossible to test
not thrilled about doing this the brute force way but it works
for clarity's sake, since .notes seems way more general than the actual purpose this element serves
was getting tired of writing stuff out
Not done yet (gotta move stuff around for SM), but this already makes the organization here clearer i think. Having FM and SM be separate clearly marked sections should make things easier to understand, since it will be very easy to say "oh this node has no feature metadata" or "oh this node isn't in the table" (biocore#314).
This makes calling this, and doing state-modifying stuff from within this function, so much less of a pain. not sure why i thought this would be a good idea back in june lmao
have things planned out, just gotta go through and refactor stuff
Closes biocore#314 for good, I think. Some bugs (biocore#272 rears its head again) and i wanna test this, but this is already a big improvement
🤔 I can't believe I just spent like 15 minutes trying to debug this lmao
wound up being a surprisingly large pain. turns out the (main) reason was that we actually shouldn't have been creating a new selected node menu in the tests; we should've just used the test data's copy. Stuff was being initialized twice, so there were 6 (not 3) s.m. fields in the <select>, and i was tearing my hair out lmao
|
... also, for reference for whoever reviews this, here's a flowchart i drew out on all of the cases possible when showing the sample metadata stuff to the user. Turns out user interfaces are really hard!
(note: i might have missed something so this might not actually be comprehensive ._.) |
|
The following artifacts were built for this PR: empire-biplot.qzv, empire.qzv, empress-tree.qzv, just-fm.qzv, plain.qzv |
| "This is a tip in the tree. However, it is not " + | ||
| "present in the input feature table, so we " + | ||
| "can't show sample presence information for it."; |
There was a problem hiding this comment.
What about a shorter version:
| "This is a tip in the tree. However, it is not " + | |
| "present in the input feature table, so we " + | |
| "can't show sample presence information for it."; | |
| "This tree tip is not " + | |
| "present in the input feature table, so we " + | |
| "can't show sample presence information for it."; |
There was a problem hiding this comment.
The reason I kept things like this was to be consistent with the other "notes" text that's shown in the alternative case (when this node is in the table) in Empress, here and here.
I'd be down to change this in the future (maybe adding another UI element next to Node length: at the top of the menu that has something like Node type: and then tip or internal), but I would prefer to keep this as is for now. (I don't feel super strongly about this, tho, so if you'd prefer we can change this up now :)
| "can't show sample presence information for it."; | ||
| } else { | ||
| wtext = | ||
| "This is an internal node in the tree. None of " + |
There was a problem hiding this comment.
Same here ... None of the descendants of this internal node in the tree ...
| #menu-box-notes { | ||
| max-width: 500px; | ||
| font-size: small; | ||
| /* TODO: remove overlap between top/bottom borders? */ |
There was a problem hiding this comment.
Yeah that would probably be nice, the borders look a bit funny IMHO. I would suggest removing them altogether ¯\_(ツ)_/¯
There was a problem hiding this comment.
I kinda like the borders, just because it makes it clear which UI elements apply to the feature metadata stuff vs. which apply to the sample metadata stuff (the distinction isn't always clear IMO without the borders, e.g. the sample metadata summarize elements). I'll try to fix the adjacent border stuff and hopefully that makes it look nicer ;P
There was a problem hiding this comment.
Actually, I spent longer than I'd like to admit trying to make the borders look the way I wanted them to and was not very successful. So... yeah, I went ahead and removed the borders 🤷♂️
I did change up a few things to still make the "groupings" of UI elements clear. Here's what things look like now:
Both feature and sample metadata available:
Just feature metadata available:
Just sample metadata available:
I think this ended up looking better than it did before, so thanks for the suggestion ;)
There was a problem hiding this comment.
That looks super slick! ✨
Thanks so much for looking into this!
Thanks @ElDeveloper for pointing this out!
Co-authored-by: Yoshiki Vázquez Baeza <[email protected]>
But make things look a lot nicer -- e.g. show note in sm section when no columns selected, remove extraneous <br />, etc.
|
Thanks @ElDeveloper! Changes should be ready for a second pass. EDIT 1: oh no the tests are failing nvm gimme a sec :( EDIT 2: OK we're good! |
|
Thanks @ElDeveloper! |
|
Thanks @fedarko!
…On (Nov-16-20|14:28), Marcus Fedarko wrote:
Thanks @ElDeveloper!
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
https://urldefense.com/v3/__https://github.com/biocore/empress/pull/432*issuecomment-728368031__;Iw!!Mih3wA!Q4muwYw6kYl56xWU5LsgErObDmHyDIYOhFSdVh0vJj0RSm3MP390VO9gSUSHhfU$
|
Closes #314. Now, clicking on a tip not in the table shows text to the user explaining the situation:
Similarly, clicking on an internal node where none of its descendant tips are in the table also shows a note:
This also refactors the node selection menu a decent amount. Mainly, I realized while working on #314 that a lot of the showing / hiding operations in the menu rely on some pretty complex logic and were done in a very ad-hoc way (this is primarily my fault lol), so I tried to make things more manageable. Now, more of the work is delegated to
makeFeatureMetadataTable()/makeSampleMetadataTable(), so life should be easier. This PR also adds some tests for the menu -- these are by no means comprehensive, but they should be a good start.While I was at it, I also fixed #272 and added a test for it.
Last change worth noting, I think, is that I removed
SelectedNodeMenu._checkTips(). When more than a few tips were not present in the tree, the toast messages shown were kind of hard to read (see below); furthermore, it looks like the tip keys rather than names were being shown, anyway, which makes interpreting these messages hard for users. Since the revamped menu now has the ability to show these sorts of warnings a lot more cleanly, I delegated this to there.Before:
After:
