Skip to content

Allow deactivation of automatic color styling #37

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 24, 2017
Merged

Allow deactivation of automatic color styling #37

merged 1 commit into from
Nov 24, 2017

Conversation

@lehni
Copy link
Contributor

@lehni lehni commented Nov 23, 2017

Closes #36

@lehni lehni force-pushed the feature/deactivate-color-styling branch from e18a628 to ea85147 Compare November 24, 2017 09:38
euvl
euvl requested changes Nov 24, 2017
View reviewed changes
src/Button.vue Outdated
? color || constants.colorChecked
: contains(color, 'checked')
? color.checked
: constants.colorChecked
Copy link
Owner

@euvl euvl Nov 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Chaining ternary operators is an anti-pattern in javascript. Never use more than one.
( 3 (!!!) here)

Copy link
Contributor Author

@lehni lehni Nov 24, 2017

Choose a reason for hiding this comment

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

Who says so? But sure, I'll change it.

src/Button.vue Outdated
? undefined
: contains(color, 'unchecked')
? color.unchecked
: constants.colorUnchecked
Copy link
Owner

@euvl euvl Nov 24, 2017

Choose a reason for hiding this comment

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

Same here.

@euvl
Copy link
Owner

euvl commented Nov 24, 2017 •
edited
Loading

I don't think that it is the right place to do it at all.
These methods start returning values of various types, sometimes undefined, etc.

I would rather put a simple check in here:

vue-js-toggle-button/src/Button.vue

Line 103 in aee66e0

backgroundColor: this.colorCurrent,

Something like:

backgroundColor: this.cssColors ? null :  this.colorCurrent,

Pretty much thats the only change that should be done to the old code.

@lehni lehni force-pushed the feature/deactivate-color-styling branch from ea85147 to b1c0879 Compare November 24, 2017 10:58
@lehni
Copy link
Contributor Author

lehni commented Nov 24, 2017

You're right. The previous commit was a left over of the approach in the first attempt + programming before coffee this morning. This is much cleaner now. Thanks!

@euvl
Copy link
Owner

euvl commented Nov 24, 2017 •
edited
Loading

Nice, great job 👍

I will publish it whenever I'll get my laptop (monday/tuesday)

@euvl euvl merged commit c580dd1 into euvl:master Nov 24, 2017
@lehni
Copy link
Contributor Author

lehni commented Nov 24, 2017

Very cool, thanks! And take your time with releasing this.

@lehni
Copy link
Contributor Author

lehni commented Nov 28, 2017 •
edited
Loading

@euvl before you deploy a new version, you need to run npm run build and commit the changes because my two PRs were both in relation to the master branch, and thus they exclude each other in their state of the dist folder. A better idea may be to define a prepare script to run npm run build automatically, and exclude the committed versions in dist through .gitignore, see https://docs.npmjs.com/misc/scripts

@euvl
Copy link
Owner

euvl commented Nov 28, 2017

Oh, dont worry, i will rebuild it anyways. I had a case on another project i was working on, where someone tried to commit a change with bitcoin miner in dist file 😄

Some people use plugin directly from Github repo, for example, in china npm sometimes is not an option (as far as i was told), i don't remember other cases, but having dist in main repo is a good thing for plugin 😄

@euvl
Copy link
Owner

euvl commented Nov 28, 2017

Btw I got my laptop yesterday night so will be able to publish this one tonight.

@lehni
Copy link
Contributor Author

lehni commented Nov 30, 2017 •
edited
Loading

@euvl friendly reminder to not forget to publish this. And I hope the new 💻 is 💯!

@lehni
Copy link
Contributor Author

lehni commented Dec 8, 2017

@euvl any chance to publish a new version? Sorry for keep bugging you...

@euvl
Copy link
Owner

euvl commented Dec 8, 2017

Wow, i completely forgot about it, will do tonight. THanks 👍 😄

@lehni
Copy link
Contributor Author

lehni commented Dec 12, 2017

👋

lehni referenced this pull request Dec 20, 2017
@euvl
v1.2.1
de0aeee
@lehni lehni deleted the feature/deactivate-color-styling branch December 20, 2017 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@euvl euvl euvl requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@lehni @euvl