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

In relation to issue #216, this patch solves the following problems: #218

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

Conversation

tmcdos
Copy link

@tmcdos tmcdos commented Mar 12, 2021

  1. v-combobox is used instead of v-select when explicitly requested by x-display or when more than 20 options are present
  2. a new selection type element is introduced - v-chip-group with v-chip items
  3. a bug in FileProperty.js was fixed (writeOnly can exist not only on the top-level field definition but also inside items when the type is array)
  4. fixed a bug in the test definition for if/then/else conditionals - neither Ajv is used/defined nor a plain validation function is provided
  5. fixed a bug in the prefilled-arrays.js test definition - the default icon map-marker does not have a matching mockup URL
  6. fixed the warning from VSlider about missing v-app in the test suite
  7. updated the version of @nuxtjs/vuetify - v1 requires fibers which needs node-gyp which needs Visual Studio on Windows (solving issue Make fibers an optional dependency nuxt-community/vuetify-module#254)
  8. updated the test snapshots

1) v-combobox is used instead of v-select when explicitly requested by "x-display" or when more than 20 options are present
2) a new selection type element is introduced - v-chip-group with v-chip items
3) a bug in FileProperty.js was fixed ("writeOnly" can exist not only on the top-level field definition but also inside "items" when the type is "array")
4) fixed a bug in the test definition for if/then/else conditionals - neither Ajv is used/defined nor a plain validation function is provided
5) fixed a bug in the prefilled-arrays.js test definition - the default icon "map-marker" does not have a matching mockup URL
6) fixed the warning from VSlider about missing v-app in the test suite
7) updated the version of @nuxtjs/vuetify - v1 requires fibers which needs node-gyp which needs Visual Studio on Windows (solving issue nuxt-community/vuetify-module#254)
8) updated the test snapshots
@albanm
Copy link
Member

albanm commented Mar 12, 2021

A large part of this is ok. But I don't think it is ok to simply replace autocomplete with combobox. If I am not mistaken this will break the current behavior in many use case, and the form will allow the user to input an invalid model (in case of select values coming from enum or oneOf).

I think combobox should be available as an option in case of a list of values filled with x-fromData of x-fromUrl, but not from enum or oneOf.

There is a pull-request (that I totally forgot to merge :() to fill select values with the examples annotation, in this case combobox is the appropriate component.

Also, could you complete one of the example in the doc to illustrate the new chip-group display ?

@tmcdos
Copy link
Author

tmcdos commented Mar 13, 2021

I have fixed the code so that it uses v-autocomplete as before (with more than 20 items), v-autocomplete when forced by x-display and v-combobox only when forced by x-display.
I also added examples to illustrate the usage.

@albanm
Copy link
Member

albanm commented Mar 15, 2021

Thanks.

Regarding the comment "Unfortunately the `v-chip-group` Vuetify component does not support label and prepend/append slots at the moment - so no field title or help tooltip will be rendered.".

I think this could be solved by wrapping the v-chip-group inside a standard v-input. This is what I do in wrapper components for third party libs (see this example). This way you can have labels, validation rules, etc on any kind of input.

@tmcdos
Copy link
Author

tmcdos commented Mar 15, 2021

IS this tip-tap wrapper available for using from consumer applications - or it is just for the VJSF documentation?

This was referenced Mar 15, 2021
@albanm
Copy link
Member

albanm commented Mar 15, 2021

See this example.

For the time being the tiptap wrapper is just an example that you can imitate. In the near future I intend to maintain a small number of wrapper components and encourage other developpers to do the same thing.

@tmcdos
Copy link
Author

tmcdos commented Mar 15, 2021

If I understand you correctly - you encourage me to use the tip-tap wrapper in the chip-group example in the docs but it is up to the users of VJSF to import and use the tip-tap in their own applications. Am I correct?

@albanm
Copy link
Member

albanm commented Mar 15, 2021

I was simply referring to the tiptap wrapper as an example of using v-input around another component in order to create uniformity of labels, etc. But the comparison stops here. Tiptap is a heavy third-party lib, this is why I do not integrate it directly in vjsf and instead create a separate component that users can load separately if they want to. v-chip-group is part of vuetify, I have no problem with using it directly in vjsf as you did in your commit (but probably with a v-input around it).

@tmcdos
Copy link
Author

tmcdos commented Mar 15, 2021

If I update my PR to reflect your suggestion (i.e. wrap v-chip-group in a v-input) - would not this break when one day Vuetify bugfix/improve v-chip-group to behave like the other input controls (adding slots and validation)?

@albanm
Copy link
Member

albanm commented Mar 15, 2021

I think vuetify will not break compatibility and the component will keep the same rendering for the same given prop. But additional props might be supported. So you should not pass the props that are ignored for now (label, etc), this way if they are not ignored in the future the behavior will not change.

@albanm
Copy link
Member

albanm commented Mar 15, 2021

Actually a better example is the color-picker.

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