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

Adding Vue-Multiselect #21

Merged
merged 9 commits into from
Aug 8, 2016

Conversation

lionel-bijaoui
Copy link
Member

Hello,

This is my first ever contribution to an open source project, so please forgive me if I make mistakes.

Since you talked about adding vue-multiselect #18, I went ahead and begun to work on that.
I'm well aware this is not ready for pull request yet, but I wanted some guidance on were to go and what to change.

Obviously, I need to update the doc, but I would also like to talk about exporting some events from vue-multiselect to be exploitable in the schema definition. For example, update is the same as onChanged, but search-change or tag don't have equivalent. Should we expose them ?

Anyway, I tried to implement all basic options (props) from multiselectMixin.js, but I need to do the same for Multiselect.vue and pointerMixin.js.

I would also like to point a larger issues: i18n & styling. Should I create separate issues, or could we talk about it here ?

  • I don't have a clear idea on how to make all strings i18n compatible.
  • I would like to remove most of the styling in composant, so that customisation is easier and don't require very specific selector or use of important.
    I can see the benefits of having a ready to work version but maybe this vision of a css-less plugin should be exploited in a "lite" version of the plugin (vue-form-generator-lite) ?

Anyway thank for your help, I really need to make this plugin the best I can !

@coveralls
Copy link

coveralls commented Aug 3, 2016

Coverage Status

Coverage decreased (-2.9%) to 85.974% when pulling 24c49d9 on lionel-bijaoui:LB_vue_multiselect into 236e9d0 on icebob:master.

@icebob
Copy link
Member

icebob commented Aug 3, 2016

Thanks! I will check your PR ASAP.

@icebob
Copy link
Member

icebob commented Aug 3, 2016

Firstly, I think it is an optional field. So please remove it from dependencies in package.json. Instead load as an external component in the HTML like bootstrap-select or ionRangeSlider fields. In field ready function, please check the neccessary lib is loaded like here: https://github.com/icebob/vue-form-generator/blob/master/src/fields/fieldSlider.vue#L30
In dev example please load multiselect like here: https://github.com/icebob/vue-form-generator/blob/master/dev/index.html#L22

If PR is merged, I will update the docs, after I released a new version. Instead of docs, please make a test file for this multiselect field. Use the fieldSelectEx.spec.js as a boilerplate.

You could write tests continuously with npm run ci
If test file ready, run npm test and check the coverate report in test/unit/coverage folder.

More comments I will add for your commits.

Thanks for you work!

For your two other questions:

  1. i18n. there are strings only in validator. It solved in this issue: localization validator #15
  2. I plan to extract CSS styles to a separated css file. After it, if someone want to styling the fields, don't load this vue-form-generator.css file and no need to use the !important. I created a ticket for this task: remove scoped styles #20

closeOnSelect: true,
// customLabel:function(){return ""},
taggable:true,
tagPlaceholder:'Press enter to create a tag',
Copy link
Member

@icebob icebob Aug 3, 2016

Choose a reason for hiding this comment

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

Please add an onNewTag event handler to the selectOptions:

selectOptions {
  ...
  onNewTag: function(...) { console.log(...); },
  ...
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm also adding onSearch (from onSearchChange) since it is a very useful function (to create dynamics option list for country and all kind of filtering).

Copy link
Member

Choose a reason for hiding this comment

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

OK, great!

@lionel-bijaoui
Copy link
Member Author

I will start the test later today, and also I will fix the linting

@icebob
Copy link
Member

icebob commented Aug 5, 2016

Great thank you!

@icebob
Copy link
Member

icebob commented Aug 7, 2016

@lionel-bijaoui when could you finish this PR?

@lionel-bijaoui
Copy link
Member Author

I'm working on it, but face a problem. I need vue-multiselect for the test to work, but I don't know how to add them for the test. I can't find in the other test a good example (only fieldSpectrum is close to this, but there a TODO).
I mean, if vue-multiselect is not loaded, pretty much nothing happen except a console.warn.
What should I do ?

@icebob
Copy link
Member

icebob commented Aug 8, 2016

Hmm, yes it's not easy. In this case, skip the unit test. If the main code is ready, commits and I merge it. After it I will try to make tests.

@icebob icebob added this to the v0.4.0 milestone Aug 8, 2016
@lionel-bijaoui
Copy link
Member Author

The .jsbeautify is useful for my beautifier (it doesn't understand .eslintrc files). I have added a basic unit test and I hope the test will work (they worked locally).

@coveralls
Copy link

coveralls commented Aug 8, 2016

Coverage Status

Coverage decreased (-1.9%) to 87.013% when pulling 54a3502 on lionel-bijaoui:LB_vue_multiselect into 236e9d0 on icebob:master.

@icebob
Copy link
Member

icebob commented Aug 8, 2016

Thanks, Travis is green. 👍

@lionel-bijaoui
Copy link
Member Author

I'm so happy right now 😄
Can i help with the CSS extraction ?

@icebob
Copy link
Member

icebob commented Aug 8, 2016

Yes, it would be very good. Further conversation from css extraction here: #20

@icebob icebob merged commit 912b833 into vue-generators:master Aug 8, 2016
@lionel-bijaoui lionel-bijaoui deleted the LB_vue_multiselect branch August 12, 2016 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants