Closed xinaesthete closed 7 years ago
Nice! Thanks for the PR.
I noticed a bug when testing out the "nesting.html" example. When you expand the "advanced settings" folder, it creates an empty cap on top of the "more" folder.
Hmm, I've been doing some fiddling with layout, had hoped I'd got it right, but will have to revise that hypothesis.
I thought that maybe if I reversed my last commit on this branch then it would be return to correct behaviour for all of the standard stuff, but not for my ImageButton (or any other stuff using heights other than Layout.PANEL_HEIGHT
) which I could then fix separately.
Unfortunately, rearranging items in the nesting example (try removing 'alive':true
from nesting.html), it looks like there was a layout bug that we hadn't spotted before. This all seems fiddlier than it should be, at some point hopefully things'll click but for now I must admit I'm slightly confused. I'll try to document test cases as I go.
OK, I believe the layout is correct now; let me know if you spot any bugs etc.
Nice, that bug looks to be all fixed up!
I have a question about the formatting.. in the old dat gui, folders are indented on the left but all have the same right edge:
but in VR we have ragged right side edges:
Would it be do-able to make the edges match like the original GUI? This also keeps the slider/buttons aligned no matter how indented they are. Maybe the buttom/sliders stay the same size, but the amount of room for the label on the left shrinks as controls are further and further indented?
One way to do it is to shift the controls over to the left so that they line up, then shrink the backing rectangle to size. This would also require some determining the max number of letters in the text label as it indents.
It certainly would be possible one way or another...
In general web design, there are a lot of examples of things like nested comment threads where things get ridiculously crushed to one side... in VR, the constraints are different (there is no side of the screen to run out of space on) and I don't necessarily believe that the current GUIVR behaviour is bad; it helps to communicate the structure to the user, as well as having the benefit of keeping the code simple. That's not to say @customlogic is wrong to see things differently, and after all, it's not my library.
It'd be good to discuss a bit more what you guys consider the scope of the library to be; on one hand it makes sense for it to be something essentially fairly simple to allow tweaking a few variables in VR, but at the same time, there's also the possibility to start making it something which is able to accommodate a wider, possibly user-extensible, range of widgets. If the latter is the case, then the kind of approach to layout suggested in @mflux's last comment might not be trivial to generalise...
For a concrete example, I'm probably quite soon going to make a "grid of image buttons" which won't conform to the "label on the left, control on the right" layout format. Another example of a widget that might be generally useful would be something like a color picker (although I suppose doing something similar to standard dat.gui might make sense for that, which would fit in the general layout style).
Anyway, for now I suggest that you accept this PR as it does fix some previously unnoticed bugs as well as adding the desired open/close folder functionality. I'm pretty sure there's nothing detrimental in it.
I like the idea of the labels shrinking as you nest more and more deeply, but maybe we should just pull this as is and add that a new issue to look into. What do you think, @mflux?
I'm surprisingly happy the way it is actually. The shift in geometry shows the flow of layout. 2D Dat gui needed to be fixed size because of its positioning on the browser window, but in VR this is kind of a moot issue. We'll probably accrue less bugs in the long run as well, by not having complex layout code.
That being said, I think we should add it to our backlog of improvements anyhow.
I don't see why you wouldn't pull this as is - it doesn't change anything about layout from my previous PR except for fixing bugs and adding the open/close function (edit: and I think making the layout code slightly clearer).
Okay, sounds good to me. Thanks again for your work on this!
Quick change to implement #82.