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

Implement .add( controller ) & its use for custom Controllers. #243

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

awelles
Copy link
Contributor

@awelles awelles commented Jun 21, 2019

See example of a custom controller in example.html.
Fixes #4.

I stripped out everything unessential. It's just add( controller ) and a couple examples.

You can include parameters when adding:

gui.add( controller, params );

Currently a parameter called liClass lives here.

Param Type Default Description
[params] Object
[params.liClass] String class name appended to controller's li row.

This was needed to avoid more special cases in assigning li row class, which is done just after controller class is squared away in GUI.js.

See example of a custom controller in example.html.
@awelles awelles mentioned this pull request Jun 21, 2019
Copy link
Contributor

@donmccurdy donmccurdy left a comment

Choose a reason for hiding this comment

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

Thanks, and sorry for the slow followup. Are you still interested in doing this PR?

} else if ( params.liClass ) {
dom.addClass(li, params.liClass);
} else if ( controller.liClass ) {
dom.addClass(li, controller.liClass);
Copy link
Contributor

Choose a reason for hiding this comment

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

Are both of these (params.liClass and controller.liClass) needed? If this li will later be set as controller.__li, could the controller add the class itself?


if (object instanceof Controller) {
controller = object;
params = property || { };
Copy link
Contributor

Choose a reason for hiding this comment

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

params doesn't take a default value normally, so is it OK to omit the || {} here?


gui.add(new KnobController(api, 'value', 0.5, 25), {
liClass: 'knobby'
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know if this is compatible with the augmentController method here?

dat.gui/src/dat/gui/GUI.js

Lines 922 to 990 in 096993e

function augmentController(gui, li, controller) {
controller.__li = li;
controller.__gui = gui;
common.extend(controller, /** @lends Controller.prototype */ {
/**
* @param {Array|Object} options
* @return {Controller}
*/
options: function(options) {
if (arguments.length > 1) {
const nextSibling = controller.__li.nextElementSibling;
controller.remove();
return add(
gui,
controller.object,
controller.property,
{
before: nextSibling,
factoryArgs: [common.toArray(arguments)]
}
);
}
if (common.isArray(options) || common.isObject(options)) {
const nextSibling = controller.__li.nextElementSibling;
controller.remove();
return add(
gui,
controller.object,
controller.property,
{
before: nextSibling,
factoryArgs: [options]
}
);
}
},
/**
* Sets the name of the controller.
* @param {string} name
* @return {Controller}
*/
name: function(name) {
controller.__li.firstElementChild.firstElementChild.innerHTML = name;
return controller;
},
/**
* Sets controller to listen for changes on its underlying object.
* @return {Controller}
*/
listen: function() {
controller.__gui.listen(controller);
return controller;
},
/**
* Removes the controller from its parent GUI.
* @return {Controller}
*/
remove: function() {
controller.__gui.remove(controller);
return controller;
}
});

... specifically — can you still remove(), listen(), and name() the KnobController normally? I don't suppose it's possible to override those methods in the subclass, which is unfortunate, but probably not this PR's problem to solve.

@amir-arad
Copy link

Hi.
I've just started using this library and seems like this PR is a huge thing for my use case.

what is needed in order for this to be published?

superkelvint added a commit to superkelvint/dat.gui that referenced this pull request Mar 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

.add( controller )
3 participants