Skip to content
This repository has been archived by the owner on Dec 3, 2021. It is now read-only.

Allow empty labels, move state CSS class to outer div. #54

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

adamdicarlo
Copy link

@cgarvis how do these changes look?

  • Allow empty labels: See the new tests, which fail on the existing code
  • Template change: This allows more styling flexibility. Specifically, I did this so I could color the main outline red or green depending on whether it's off or on. It's a lot easier to put the border on the outer element.
  • Updated the build (btw I think it was already out of date).

@adamdicarlo
Copy link
Author

I'd suggest bumping major version if you merge this, due to the CSS change.

@adamdicarlo
Copy link
Author

Ah, did you want me to remove the "Update build" commit from this PR?

@cgarvis
Copy link
Owner

cgarvis commented Jan 7, 2015

Yeah don't have the build commit. I do that on a release on github. That way the built js is what is stable.

if (!attrs.onLabel) { attrs.onLabel = toggleSwitchConfig.onLabel; }
if (!attrs.offLabel) { attrs.offLabel = toggleSwitchConfig.offLabel; }
if (!attrs.knobLabel) { attrs.knobLabel = toggleSwitchConfig.knobLabel; }
if (angular.isUndefined(attrs.onLabel)) { attrs.onLabel = toggleSwitchConfig.onLabel; }
Copy link
Owner

Choose a reason for hiding this comment

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

Is there a specific reason you switched these? If I remember correctly, there is a reason we do this.

Copy link
Author

Choose a reason for hiding this comment

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

Yes; because it was impossible to specify an empty label via a directive attribute.

foo = '';
!foo;  // true
angular.isUndefined(foo);  // false

You can try the new tests for empty labels on master and watch 'em fail.

@adamdicarlo
Copy link
Author

(Will re-push without the 'Update build' commit tomorrow)

@adamdicarlo
Copy link
Author

Re-pushed without the Update build commit.

@adamdicarlo
Copy link
Author

Is this merge worthy? Anything I can do to clean it up?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants