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

use parent events and value on checkbox #5

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

Conversation

davidmeirlevy
Copy link
Collaborator

  1. allows user to use "v-model" with all it's modifiers (because value binding and all related events are valid).
  2. reduce code, because now the component doesn't need to emit the change event manually.
  3. specify props types.
  4. add ability to add "name" attribute to the checkbox - needed for some forms.

1. allows user to use "v-model" with all it's modifiers (because value binding and all related events are valid).
2. reduce code, because now the component doesn't need to emit the change event manually.
3. specify props types.
4. add ability to add "name" attribute to the checkbox - needed for some forms.
@JonathanDn
Copy link
Owner

  1. I do not really see a use case for this. There is no point in passing to the toggle button a string it only is supposed to receive a boolean - isActive

  2. Working with Input value instead of isActive is interesting, Could you elaborate on what value the $listeners provides, what does it do?

3 is perfectly clear and I agree on it being a better addition.

  1. Can you elaborate on this, why should I need a name attribute - for accessibility?

@davidmeirlevy
Copy link
Collaborator Author

  1. I changed the "isActive" to "value" because when you use "value" prop name - you can also pass it as v-model, so users can use this component using "v-model".

  2. you don't need to manage the input events and triggers at all.
    The user can register to all of the input native events, such as focus, change, input, etc..
    Also, when users are using "v-model", they expecting the "input" or "change" events (depends if they are using "lazy" modifier or not).

  3. thanks.

  4. the "name" is used for native html forms.
    There are several cases that it's recommended to add name attribute to inputs.

@JonathanDn
Copy link
Owner

Alright so to wrap up:

  • isActive is changed to value prop (please update the readme documentation according)

  • because of that change consumers of the component can now use v-model - (Can you think of where in the documentation this information would be useful, maybe as a code example? if so feel free to add it to the readme)

  • The docs are addressing the custom events - please adjust them to the relevant 'input' and 'change' events so it would be clear for consumers of the component.

  • same about 'name' prop now exposed as part of the API - needs to be documented.

@davidmeirlevy
Copy link
Collaborator Author

Readme file has changed. please review.

Copy link
Owner

@JonathanDn JonathanDn left a comment

Choose a reason for hiding this comment

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

  • Reviewed the readme updates - added a few comments and suggestions

README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@davidmeirlevy
Copy link
Collaborator Author

readme updated. please review again.

@davidmeirlevy
Copy link
Collaborator Author

@JonathanDn ?

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.

None yet

2 participants