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

Add color label new prop and minor refactor #132

Conversation

guastallaigor
Copy link

@guastallaigor guastallaigor commented Mar 27, 2020

This closes #102 .

This PR adds a new prop called colorLabel which is basically the same as the prop color, but it changes the color of the label instead of the background.
Since it's using the same logic (computed properties) as the color prop, I decided to create a new component specific for labels called ButtonLabel.vue (since the other component is called Button.vue), and to reuse the logic I created a mixin that is being used by both components. That way all the computed properties and some props are being reused and there isn't no code duplication.
I had to just remove the consts and changed to data properties, so that the colors are different in between those two components.
For me to reuse those computed properties in a mixin between those two components, the prop color being used differently in each component.
The prop color inside ButtonLabel.vue is being used for the label colors (font color).
The prop color inside Button.vue is being used for the background color of the toggle.
The v-switch-label class was moved to the children component.
The slots was removed for now since there wasn't no used to them.
I also removed an isBoolean import that was not being used.
I tested in my browser by importing the dist into another Vue project.
The README.md was also updated.


This is the best I could do for this to be as cleanest as possible, and to have no code duplication.

image
image

Please let me know what do you think of this.
Thank you.

}
},
switchColor: {
colorLabel: {
Copy link
Owner

Choose a reason for hiding this comment

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

Is'nt it a breaking change?

</template>
</label>
</template>

<script>
import { isString, isObject, isBoolean, has, get, translate, px } from './utils'
import { isString, isObject, has, get, translate, px } from './utils'
import generalMixin from './general-mixin'
Copy link
Owner

Choose a reason for hiding this comment

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

I think we use camel case for file names

@@ -239,30 +184,31 @@ export default {
},
data () {
return {
toggled: !!this.value
toggled: !!this.value,
defaultColorChecked: '#75c791',
Copy link
Owner

Choose a reason for hiding this comment

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

Not sure i understand this bit, seems you have this defined in another file aswell. Also what was wrong with having constants?

@guastallaigor
Copy link
Author

Thank you. I will separate this into multiple PRs when I get some time.

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

Successfully merging this pull request may close these issues.

Feature Request: Font Colours
2 participants