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

Streamlining of schema #481

Merged
merged 13 commits into from
Sep 17, 2018

Conversation

lionel-bijaoui
Copy link
Member

@lionel-bijaoui lionel-bijaoui commented Aug 3, 2018

  • Please check if the PR fulfills these requirements
  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
    The ways schema is written is changed to make it easy to create custom fields and write documentation.
  • What is the current behavior? (You can also link to an open issue here)
    For any given field, it's option are not really enforced.
    Depending on the field, top level key are a mix of this and whatever the (custom) field decide to put in.
    Also, other options can be put into ***Option (e.g. radiosOptions, selectOptions, pikadayOptions).
    In the end, it make it hard to guess where to put options when creating custom fields and prevent some ease of use.
  • What is the new behavior (if this is a feature change)?
    With this PR, top keys are restricted to this list :
const allowedKeys = [
    // Minimal
    "type",
    "model",
    // Identity
    "id",
    "inputName",
    // Texts
    "label",
    "placeholder",
    "hint",
    "help",
    // Modifiers
    "featured",
    "visible",
    "disabled",
    "required",
    "readonly",
    "validator",
    // Other options
    "styleClasses",
    "labelClasses",
    "fieldClasses",
    "fieldOptions",
    "values",
    "buttons",
    "attributes",
    // Getter/Setter
    "get",
    "set",
    // Events
    "onChanged",
    "onValidated"
];

Nothing actually stop you from adding anything else to this list, but be aware that it will be frown upon and will prevent your custom field from being part of the "official" list.
fieldOptions was created to replace ***Options. This mean that any option not in the list must go under this key.
This is the first step toward the future V3 and more tools (builders, etc).

  • Does this PR introduce a breaking change?
    There are multiple breaking changes.
    You will need to modify your schema to fit the new system.
    If you are a custom field creator, you need to change the logic and keys to fit.

multi, multiple and default are deprecated. Their related functions will be removed when the new group system will be created in a following PR.

  • Other information:
    Other changes include making inputName, placeholder, disabled, required, readonly, fieldClasses, fieldOptions and values as computed of fields. It make these options directly available to field (schema.readonly become readonly). They can also all be defined with a function that return their value if needed.

@zoul0813
Copy link
Member

zoul0813 commented Aug 3, 2018

fieldClasses and styleClasses are different. fieldClasses defines the CSS Class used on the <input />, and styleClasses defines the class used on the form-group that contains the component.

This allows you to define 'form-control' class on the input, and 'col-md-4' on the form-group that contains your field, for layout control.

@zoul0813
Copy link
Member

zoul0813 commented Aug 3, 2018

why is 'default' being deprecated? Do we not allow the form schema to define default values for fields anymore (or was this not used for that?).

@lionel-bijaoui
Copy link
Member Author

My bad, I did not understand that. I will add styleClasses back.

default is totally independent from the internal workings of VFG. It use createDefaultObject that is use to transform a model before assigning it to VFG. It's the same thing with multi (mergeMultiObjectFields).

If you want to add default, nothing actually stop you (I did not implement any restriction for this reason). Maybe I will export those function into an "utils" lib outside of the main repo. Maybe this could be defined as "plugins" in the future.
My main goal is to simplify schema, VFG "engine" and future development by cutting some functions.
If you think this is not a good idea, we can discuss it of course.

@zoul0813
Copy link
Member

zoul0813 commented Aug 3, 2018

I assumed default was just a default value for a field if the model did not define one.

@icebob
Copy link
Member

icebob commented Aug 4, 2018

Hi guys, currently I have no time to review this PRs. I don't want to block the development, so don't wait for me, I trust you :) Thanks.

@lionel-bijaoui
Copy link
Member Author

lionel-bijaoui commented Aug 9, 2018

I'm having a hard time with the test, but I think I'm finally starting to get them to work.
Unrelated, here is another view of the keys and where they are used mainly.

const allowedKeys = [
    // used in form group only
    "type",
    "model",
    "label",
    "hint",
    "help",
    "featured",
    "visible",
    "styleClasses",
    "labelClasses",
    "buttons",
    // used in both
    "id",
    "disabled",
    "required",
    "readonly",
    "fieldOptions",
    // used in fields only
    "inputName",
    "placeholder",
    "validator",
    "fieldClasses",
    "values",
    "attributes",
    "get",
    "set",
    "onChanged",
    "onValidated"
];

disabled, fieldClasses, fieldOptions, inputName, placeholder, readonly, required and values are available as computed in the fields. They are using the same logic from formGroup so they can be a Function, or any value and they have fallback.

Options Fallback Type
disabled Boolean
fieldClasses Array
fieldOptions Object
inputName String
placeholder String
readonly Boolean
required Boolean
values Array

EDIT: my bad for accidentally closing the PR

@lionel-bijaoui
Copy link
Member Author

The test where a real pain to get to work, but I was able to make it.
I had to skip some test, because I was already taking a lot of time on this.
Good new is that I also have unskipped some test.
With manual verification, everything seem to work, but please test it on your own.

I have added labelClasses to the authorized keys.

If this is ok with you, I will stop here on this. I will do the documentation later.
@zoul0813 I'm counting on you to check and validate my work. If I didn't do anything too stupid, please validate and I will merge.

After that I will start to work on a better way to create group alongside fields (and group in group in group, because why not ?). I have an idea on how to do it, but I need to get my hand dirty and face real code !

@lionel-bijaoui
Copy link
Member Author

At least ! Travis is fixed !

@lionel-bijaoui lionel-bijaoui mentioned this pull request Aug 16, 2018
@lionel-bijaoui lionel-bijaoui merged commit c0b65b5 into vue-generators:v3 Sep 17, 2018
@lionel-bijaoui lionel-bijaoui deleted the lb_streamlined_schema branch September 17, 2018 09:48
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.

3 participants