Add functionality to disable folders

[Degubi]

Basically copied it from the Controller class to the GUI class.
The only difference is that I made the folder autoclose when disabling it.

At the moment I use this external function to do the same, but I think it makes sense to have this functionality as part of the GUI class instead:

function setFolderEnabled(folder, enabled) {
    const folderElement = folder.domElement;
    folderElement.style.pointerEvents = enabled ? 'auto' : 'none';
    folderElement.style.opacity = enabled ? '1.0' : '0.5';

    if(!enabled) {
        folder.close();
    }

    return folder;
}

[georgealways]

Hi there, thanks for this. So we just crossed over 30kb un-gzipped and I'm starting to get a little self-conscious about file size. I'm already a little bothered by the fact that Controller and GUI duplicate code for show/hide.

If we were to do this, I'd want an elegant way for these classes to share code and styles. At one point I tried making them both inherit from some base class to share fields like domElement and hide() / show() but it made the code a little too clever / hard to read. Maybe someone could do a better job of that than me.

[Degubi]

Hi @georgealways! Thank you for checking out this PR!
I added a new commit, which implements a simple function based implementation sharing between Controller.js & GUI.js for the show & disable functionality.
I opted for this simple approach instead of an inheritance based one because I don't think using a base class is useful for these classes.

About sharing the styles: I don't really know how the scss syntax works :|
I can take a look at the weekend at how the styles could be shared.

[Degubi]

@georgealways
I added a new commit where I renamed hover.scss to shared.scss.
I added a new mixin called 'disabled' and now use it instead of the copy-pasted approach.

[georgealways]

Are you able to see if these new mixed-in methods appear correctly on the API page?

[Degubi]

These images are from the API.md previewed in VSCode. Both 'enable' and 'disable' are there for GUI and Controller too

[georgealways]

Thanks again for looking at that! Just had a chance to try this PR myself, overall looking great.

Thoughts on the functionality itself:

• I don't think disable should automatically close a GUI. Let's leave that up to the dev.
• The entire GUI shouldn't be transparent if the root is disabled: only the text and controllers should dim. I can lend a hand here if that gets hairy.

I like your change on hover => shared.scss

• I think @disabled should be renamed to @can-disable to match the hover functions (but maybe they won't share too much CSS if disabled folders and controllers are styled differently).

On utils/guiUtils.js I have some thoughts around naming mostly:

• The entire folder is basically GUI utils, so I wonder if this could be more descriptive.
• disableGUI/showGUI — Could be disableItem to be more accurate, or maybe just disable/show?
• The JSDoc you've done here is great, but it might be a little above and beyond since these functions aren't exposed. Then it might feel a bit better to call the gui parameter something like item.

This is petty on my part, but internally, I wish the helper methods were (enable/show) or (disable/hide). The fact that there's one "positive" and one "negative" bugs me. I think you're following my current pattern though, so that one's on me :)

Let me know what you think. Appreciate your help! -g

[Degubi]

• I don't think disable should automatically close a GUI. Let's leave that up to the dev

For me a 'disabled' folder means the user can't open it -> the user can't interact with any of it's fields. That's why I added an automatic close to the disable method. Maybe converting it to a parameter named 'closeOnDisable' with a default value of false is a good compromise?

• The entire GUI shouldn't be transparent if the root is disabled: only the text and controllers should dim. I can lend a hand here if that gets hairy

In this case yes, I might need help with this!

• I think @disabled should be renamed to @can-disable to match the hover functions

Makes sense! Renamed the mixin to '@can-disable'

• it might feel a bit better to call the gui parameter something like item

This makes sense to me! So then

• Could be disableItem to be more accurate

'disableItem' and 'showItem' names should also make sense to me! Also

• The entire folder is basically GUI utils, so I wonder if this could be more descriptive

maybe 'itemUtils'? Because now it should contain 2 functions ('disableItem' and 'showItem')

• The JSDoc you've done here is great, but it might be a little above and beyond since these functions aren't exposed

Yes, I left them there so that I could use autocompletion & typing checks, removing it makes sense because these are just internal functions.

I wish the helper methods were (enable/show) or (disable/hide).

Makes sense to me! I can rename it if you want to! (Didn't do this one yet)

[georgealways]

Thanks so much! Looking awesome.

• I don't think disable should automatically close a GUI. Let's leave that up to the dev

For me a 'disabled' folder means the user can't open it -> the user can't interact with any of it's fields. That's why I added an automatic close to the disable method. Maybe converting it to a parameter named 'closeOnDisable' with a default value of false is a good compromise?

Well we have the chaining: gui.disable().close() feels better than gui.disable( true, false ). And I think it's more conventional to have the controls still visible while disabled.

I also feel like folders should still be foldable/unfoldable in their disabled state... what if GUI.disable() just loops through its children and calls .disable() on each controller? That would also fix the issue of the semi-transparent root. But if you wanted disabled controllers to stay disabled when you enable the parent folder (🙄) you'd be out of luck. Let me know what you think of that idea.

Otherwise, I'll take care of semi-transparent root. And I'll take care of show/hide rename for sure.

[FrostKiwi]

Wanted to add, that this is also a feature request from me.

I have some toggles, that enable or disable features. So the user doesn't get confused about toggling things which are disabled, I .disable() elements. I would love to have the same for entire folders, as right now I just disable all child elements and the associated code doesn't look very elegant.

@Degubi Big thanks for taking on this challenge.

[Degubi]

Well we have the chaining: gui.disable().close() feels better than gui.disable( true, false )

Yep, the chaining solution definitely looks better!

But if you wanted disabled controllers to stay disabled when you enable the parent folder (🙄) you'd be out of luck

Yeah, this is why I decided to make disabled folders not openable/closable in the first place :|
(Which is why i also autoclose folders when disabling them)
Tbh, I'm not sure anymore what would be the best solution here...
If i wanted to disable all controllers in a folder I could just loop through all of its child controllers and disable them.
For me disabling a folder means disabling the whole 'group', which means I can't even access any of its elements.

[georgealways]

Yea, I'm having some trouble with it myself. I took a crack at it in this branch: dev...disable-folders

I think it works / looks as intended, but you can still tab into inputs inside a disabled folder. That's what the $disable property of controllers is for: marking the actual form element with a disabled HTML attribute.

There's no way to add that attribute to the container element and have it filter down unfortunately. GUI could try to apply that attribute to children on disable, but then we run into a dual state issue: how to remember which controllers weren't enabled when re-enabling the folder?

Stepping back, this feels like a lot of complexity when gui.controllers.forEach( c => c.disable() ) exists, but I'll keep thinking on it.