Disabled Bellows should prevent close behaviour

[marlowpayne]

Status: Ready for Review
Owner: @marlowpayne
Reviewers: @scalvert @kpeatt @tedtate @Helen-Mobify

Changes

• To me, adding the bellows--is-disabled class to a bellows__item should mean that we can start with the Bellows item either open or closed. Currently this disabled class only affects the opening behaviour of a Bellows item, and thus if an item starts open the behaviour pattern is open -> permanently closed, where it should simply be permanently open.
• I have simply added the same check for the disabled class from the open method to the close method, and wrote a new test for it

Todos:

• Engineer +1

Feedback:

none so far

How to Test

• Checkout the branch
• Run grunt test and grunt test:browser

[kpeatt]

Logic makes sense to me.

[scalvert]

I wonder if now we should have an _isOpen function to encapsulate this?

[marlowpayne]

A good option if there's any more usage for it, but at the moment I think the logic is trivial enough to not need any more abstraction.

[scalvert]

So I don't feel like it's abstraction, but rather will provide better readability. We're using this same check in two places, with a ! to invert the result. I would rather see it in one place. If we had done this before, the disabled check would have automatically been applied to both without the need for this change. Get my meaning?

[marlowpayne]

I see your reasoning @scalvert but I don't really understand why we should encapsulate here, since a single function such as _isOpen won't adequately capture the logic. If we would have lumped in the disabled check with the opened check then negating both of them would be the wrong logic.

[scalvert]

I do agree that in this case you can't really encapsulate this while incorporating the negation, but my guess is there's still a way. I think we want to go for readability whenever we can, and the point I'm trying to make here is that reducing conditional logic into a simple method with semantic meaning can go a long way.

[marlowpayne]

Totally agree.
Would something like if (!_isOpen || _isDisabled) be good enough? Or would we need something like if (_isClosed || _isDisabled)?

[scalvert]

I think if we did:

if (!this._isOpen() || this._isDisabled())

would work.

[marlowpayne]

Pushed up those helper methods. @scalvert could you please have another look and let me know what you think. Thanks!

[scalvert]

@marlowpayne OK I made a few small changes to clean things up a bit. Take a peek and if you're OK with the changes we can merge.

👍 from me at this point!

[marlowpayne]

Looking nice and clean. Thanks @scalvert ! Are you handling version bumping and tagging?

[scalvert]

Do you want to? How about I walk you through it?