Added a toggle method

[micky2be]

Here a proposal to fixe issue #22

It will take a boolean as first argument

true => call the show method false => call the hide method !boolean => call the appropriate method to swap the status

The second argument (data) will be carry other the event call.

[ronkorving]

Regarding the implementation, I better respond here. You write in your PR:

true => call the show method false => call the hide method !boolean => call the appropriate method to swap the status

But if not boolean, you shouldn't toggle with the current state. I think @bjornstar pointed out earlier the need for truthy/falsy tests. Boolean should not be required to indicate your intention. If you want it to toggle with the current display state, simply don't pass an argument (in other words: test for undefined instead of the boolean type).

[micky2be]

The problem is with the data we wanna pass as second argument. How should you call the toggle method then?

wuiDom.toggle(undefined, { data: true });

[micky2be]

Beside, what about null? What's the real point of accepting truthy/falsy? The developer should deal with its data rather than the API.

What really means undefined? Because of the empty argument case? What is you give some silly function as argument that returns undefined?

I would rather have a strict API that will force developer to send a proper value than having those cases to deal with.

[micky2be]

Reading my previous comment made me think that if we get rid off the data I could check the number of arguments. If none I toggle, else I parse the first argument for a truthy/falsy test. So passing null or undefined would be consider as false

But that's only if we remove the data from there.

[bjornstar]

I am all for removing data

[micky2be]

Let's do it then. But I don't wanna hear anybody crying after that.

[bjornstar]

They are free to use the old version for eternity. BWahahhaha

[micky2be]

PR updated

[bjornstar]

I like it!

[ronkorving]

Please don't publish this before talking to our Dofus team. @baptistebrunet @cstoquer @stouf etc

[micky2be]

I would like to get more feedback indeed. (Always)

[stouf]

To me, a toggle method should swap something to its opposite. Allowing a boolean argument that would force whatever is semantically incorrect IMO. Especially when there are show and hide methods existing.

[bjornstar]

http://api.jquery.com/toggle/ http://mootools.net/docs/more/Element/Element.Shortcuts http://yuilibrary.com/yui/docs/node/node-view.html http://api.prototypejs.org/dom/Element/toggle/

[stouf]

To switch from one setting to another. The term toggle implies that there are only two possible settings and that you are switching from the current setting to the other setting.

http://www.webopedia.com/TERM/T/toggle.html

The fact that others use "toggle" in a wrong way (semantically) is not an argument for us to do the same IMO. Then, it's on you guys to decide if it's important or not to be that anal about it ;-)

[micky2be]

@stouf If you don't want to force it, just don't. Btw have you read the Readme:

The method can also receive a boolean as argument to avoid complicated code for a hide/show logic.

if (shouldDisplay) {
    myDiv.show();
} else {
    myDiv.hide();
}

Now becomes:

myDiv.toggle(shouldDisplay);

APIs are made to help. Allowing an argument just mean something like 'toggle to show' or 'toggle to hide', nothing wrong with that.

[micky2be]

@Wizcorp/developer last call

[cstoquer]

why not setVisibility ?

[polco]

toggleDisplay() +1. I wouldn't know what toggle actually toggles otherwise.

[ronkorving]

Too many voices to ignore here fellas.

[carlesvallve]

how about toggleHornyness() ?

On Thu, Jun 12, 2014 at 10:05 AM, Ron Korving notifications@github.com wrote:

Too many voices to ignore here fellas.

— Reply to this email directly or view it on GitHub https://github.com/Wizcorp/wui-Dom/pull/23#issuecomment-45819311.

[carlesvallve]

setVisible(boolean) is what i like

On Thu, Jun 12, 2014 at 10:40 AM, Carles Vallve cvallve@wizcorp.jp wrote:

how about toggleHornyness() ?

On Thu, Jun 12, 2014 at 10:05 AM, Ron Korving notifications@github.com wrote:

Too many voices to ignore here fellas.

— Reply to this email directly or view it on GitHub https://github.com/Wizcorp/wui-Dom/pull/23#issuecomment-45819311.

[maxwan]

Question, why do toggle need to have the boolean parameter? Isn't this what we are suppose to do? .show() - show .hide() - hide .toggle() - hide if visible or show if invisible

supplying the parameter is like redundant as it is doing the same thing as the previous two and toggle should only act like a "switch" right? on or off. Just my opinion, ignore me if you don't like it. Thanks.

p/s: toggleDisplay() is clearer on what it suppose to do.

[ronkorving]

The point of passing a boolean is that sometimes you hold a variable that directly indicates whether something should be visible or not.

var canPurchase = coins >= price;
buyButton.setVisibility(canPurchase);

[stouf]

Repeating myself, but I totally shared the opinion of @maxwan .

However, the need @ronkorving has just pointed out is obvious. But such a function should not be called toggle then IMO. Better call it setVisible or setVisibility instead, which is semantically a better choice to me.

[bjornstar]

So this is taking way longer than it needed to. Who is going to make the decision here? I will be more than happy to make the decision if no one else wants to.

[micky2be]

It's gonna be toggleDisplay with an argument to force the display. So just a rename from this PR. I don' t see the point to have a toggling method + a setting method ( + show and hide).

Thank you for your feedback all.

[bjornstar]

Thanks, I'm eager to start using this.

[micky2be]

By the way guys, I needed feedback mostly about the fact that won't be able to use data anymore on show and hide. Thank you anyways.