Skip to content

Conversation

@zoul0813
Copy link
Member

@zoul0813 zoul0813 commented Mar 9, 2018

created a new "attributes" directive that binds input/wrapper (with support for other named sets) attributes to elements to support dynamic attributes (such as title="Tooltip Text" data-toggle="tooltip", etc)

  • added directive to abstractField
  • implemented directive in various core fields as an example of usage
  • updated basic/app.vue with samples

zoul0813 added 2 commits March 9, 2018 09:45
…upport for other named sets) attributes to elements to support dynamic attributes (such as `title="Tooltip Text" data-toggle="tooltip"`, etc)

* added directive to abstractField
* implemented directive in various core fields as an example of usage
* updated basic/app.vue with samples
@coveralls
Copy link

coveralls commented Mar 9, 2018

Coverage Status

Coverage increased (+0.2%) to 89.916% when pulling 5fdf339 on zoul0813:v3-features/dynamic-html-attributes into 64d0421 on vue-generators:v3.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 89.3% when pulling 25e7efb on zoul0813:v3-features/dynamic-html-attributes into 64d0421 on vue-generators:v3.

Copy link
Member

@icebob icebob left a comment

Choose a reason for hiding this comment

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

Please add relevant tests too.

Copy link
Member

@lionel-bijaoui lionel-bijaoui left a comment

Choose a reason for hiding this comment

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

I'm sorry, I don't really understand the use of this feature.

It is to add attribute of a field, but why ? What is the use case ? It will need to be added to every existing, future and custom fields, so I need more information.

Thanks !

@zoul0813
Copy link
Member Author

@lionel-bijaoui it allows you to add custom attributes to the components elements. So, for example, you can add data-toggle="tooltip" title="Custom Title displayed via Bootstrap Tooltip" to the <input /> element of fieldInput, or you could add it to the .wrapper element ... you'll also be able to add custom attributes to the label element and other various things in the future.

I created this as a directive, attached to abstractField, so that it is as reusable as possible ... you just have to use v-attributes="namedElement" in the template to wire up custom attributes to your component. I'm unaware of another way to accomplish this ... afaik, you can't bind attributes to an element in VueJS without being explicit, so this was the next best thing I could think of.

I've already updated all of the vfg-core components to support it with this PR. Optional components would still need to be updated, but my understanding is that those will be separated out into their own packages and most of them will likely get a pretty big overhaul so I'm leaving those alone for now, and focusing on "core".

Example field schema:

{
  type: "input",
  inputType: "text",
  model: "name",
  label: "Name",
  attributes: {
    wrapper: { "data-toggle": "collapse" },
    input: { "data-toggle": "tooltip", "title": "Some custom tooltip title" },
    label: { "custom-attr": "custom-value" }
  }
}

This has been requested a few times, and seems like a fairly generic feature to have available ... allowing developers to associated any misc attributes they want with elements contained within VFG, which would allow them the ability to attach third-party libraries like Bootstrap Tooltips, etc without writing custom components.

@lionel-bijaoui
Copy link
Member

lionel-bijaoui commented Mar 14, 2018

Ok, I didn't think about third party libraries. I like your solution (very elegant and simple), but something bugs me, and I don't know what.
Maybe it should belong under fieldOptions since it is not universal (some fields have wrapper, other don't) ?
I wish we could avoid the directive. Maybe a custom attribute (data-vfg-wrapper, data-vfg-input) that we could target on mounted ?

@zoul0813
Copy link
Member Author

@lionel-bijaoui the directive seemed like the best way to implement this. I had considered using something like mount, however, the directive allows us to bind to updates as well so the attributes become reactive... without having to handle that manually on our end.

Is there any particular reason you dislike the directive approach? I'm fairly new to Vue and am not sure if there is an issue with directives.

As for the "fieldOptions" bit, I'm open to suggestions here ...

schema.attributes.input["data-toggle"] vs schema.fieldOptions.attributes.input["data-toggle"] isn't much different ... the problem is supporting custom attributes on the various elements within a field component (wrapper, input, label, error output, buttons, etc, etc, etc).

Currently, attributes.input, attributes.wrapper, attributes.label are supported ... for components which have support for those elements. It defaults to attributes.input, so if you just supply an attributes object with custom attribute names, it will map them all to input element (though this logic may create problems in the future if it's not properly documented, as it will likely create confusion?).

I had also considered something like

{
  type: "input",
  inputType: "text",
  inputAttrs: { "data-toggle": "tooltip" },
  wrapperAttrs: { "data-toggle": "collapse" },
  labelAttrs: { "custom-attr": "custom-value" }
}

But wasn't keen on this approach as it added numerous top-level properties to the schema object, ... though these could be placed inside of fieldOptions ... but then, that breaks each of the custom attributes out into their own objects without a clean "parent"...

Open to any suggestions you may have ...

* v3:
  added brackets to conditional
  cleaned up the "clean up"
@lionel-bijaoui
Copy link
Member

lionel-bijaoui commented Mar 14, 2018

@zoul0813 I didn't think about reactivity, so I agree with you solution with the directive.
As for the option, here is my plan.
Make top-level properties available for all fields, no exception. Add fieldsOptions which replace the different ***Options (selectOptions for example).
fieldsOptions become the only place to put field specific parameters and options. It harmonize the way you configure a field and allow us to add a computed (named options for example) in abstractField.js for easy access to the field specific options.

Since attribute is not always the same, I'm thinking about putting it here, but I'm not sure to be honest. It could become a top-level property alongside type or model.

TL:DR I hesitate between

{
  type: "input",
  model: "name",
  label: "Name",
  fieldsOptions: {
    inputType: "text"
  },
  attributes: {
    wrapper: { "data-toggle": "collapse" },
    input: { "data-toggle": "tooltip", "title": "Some custom tooltip title" },
    label: { "custom-attr": "custom-value" }
  }
}

or

{
  type: "input",
  model: "name",
  label: "Name",
  fieldsOptions: {
    inputType: "text",
    attributes: {
      wrapper: { "data-toggle": "collapse" },
      input: { "data-toggle": "tooltip", "title": "Some custom tooltip title" },
      label: { "custom-attr": "custom-value" }
    }
  }
}

@zoul0813
Copy link
Member Author

Personally, I think the top-level property is best, as it is a "common" functionality and is not dynamic or dependent upon the field ... fieldOptions is a great solution to things like selectOptions as it will allow those "misc options" to live in one place, and as you said, we can handle those in abstractField more easily ...

My biggest issue though, is the structure ... it seems "off", and I'm not sure how to make it "right".

{
  // ...
  attributes: {
    wrapper: { ... },
    input: { ... },
    label: { ... },
    whatever: { ... }
  }
}

I really don't like the wrapper/input/label/whatever structure ... but I'm struggling to find a better way to present this so that developers can attach custom attributes to the various 'important' elements of a field ... obviously, the input element is the most critical ... but the wrapper, label, and other various bits are also important when it comes to creating really dynamic forms with loads of features. Do you have any experience with similar projects who handled custom attributes in a different way?

@lionel-bijaoui
Copy link
Member

lionel-bijaoui commented Mar 14, 2018

Yes, you are right, it is a top-level attribute.
As for your question, I like your solution, it strike a good balance between structure and freedom. Most fields will use 'input' and 'wrapper'. It is very clear and easy to add another element if needed.
You should work on some tests, it may help you find issues with the solution but I like it very much 😃

@zoul0813
Copy link
Member Author

@lionel-bijaoui I've already added some tests - do you feel that more are needed? If so, could you assist with that?

On a side note ... Hopefully, we can address the formGenerator's template structure in v3 ... and move the .form-group block out of formGenerator and into abstractField, making .form-group the "wrapper" for every field ... eliminating the need for some fields to have a "wrapper", but that is a discussion for another thread :)

Copy link
Member

@lionel-bijaoui lionel-bijaoui left a comment

Choose a reason for hiding this comment

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

@zoul0813 Sorry, I just saw @icebob block the PR for the test and I did not see that you provided them. I'm all green for the merge !

@zoul0813 zoul0813 merged commit e21a0de into vue-generators:v3 Mar 14, 2018
@zoul0813 zoul0813 deleted the v3-features/dynamic-html-attributes branch March 14, 2018 16:54
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.

4 participants