CroudTech / vue-quill

Quill component for vue
MIT License
121 stars 22 forks source link

Update to Vue 2.x and Quill 1.x #13

Closed Elhebert closed 7 years ago

Elhebert commented 8 years ago

This is an updated version of vue-quill for VueJS 2 and Quill 1.1.x

Quill remove the setHTML and getHTML in v1, thus I used a "hack" : this.editor.root.innerHTML.

Instead of using a toolbar <slot> I preferred the object version, since it's way easier to use and change.

Also, a big issue was the "one-way-down" binding of Vue 2, thus it's needed to use events to pass the data from the child to the parent. (see README) A custom store could be use for a cleaner solution, but that'll require more tweak to do for the user.

For the filters, I'm not sure if the render will still work properly (due to the {{{ }}} deprecation). Not sure if you can use a filter in v-html.

Still need to do some test and tweaks, but most of the work is done. I'm PRing it while it's still wip to open a discussion about the changes I made, if you have any questions or remarks, I'm here.

BrockReece commented 7 years ago

@Elhebert Thanks for this.

Sorry for the slow reply, I have been away for a while. I think I might create a new branch for these changes as I don't want to pull the rug from anyone still depending on the legacy versions and then maybe bump the npm version to 1.x.x in the branch.

I will have a proper read through this later when I get a bit more time and let you know my thoughts

Thanks again. Brock

Elhebert commented 7 years ago

@BrockReece No problem, I've been working on other part of my code, thus I didn't really continue working on this. I still need to test the filters and update the examples, I'll try to do that this week-end.

Doing a new branch is a smart move, if you create the branch I'll edit the PR to merge in it.

If you have any questions, I'll do my best to answer them.

BrockReece commented 7 years ago

@Elhebert, once again, thanks for your help with this.

I have created a new branch (vue2) for you to move it over to.

BrockReece commented 7 years ago

Hi @abruere

I have created a vue2 branch that I have been testing for the last month or so. The changes were loosely based on this PR. If you are happy with that branch I will merge it in to master and tag it up.

Cheers Brock

abruere commented 7 years ago

Hi @BrockReece

I noticed vue2 branch indeed, but I must confess lodash dependency spurred me to use @Elhebert branch, since lodash made my webpack chunk more than 1MB heavier (lodash is not included in my vue app).

This aside, vue2 branch merge into master would be very nice.

Would you mind replacing single _.defaultsDeep with this kind of method to remove lodash dependency?

https://github.com/Elhebert/vue-quill/blob/0a49aa9d512a191e4d676e9c687d52023d749673/src/Quill.vue#L101-L110

setOptions(defaults, properties) {
  for (let property in properties) {
    if (properties.hasOwnProperty(property)) {
      if (typeof properties[property] === Object) {
        defaults[property] = this.setOptions(defaults[property], properties[property]);
      } else {
        defaults[property] = properties[property]
      }
  }
}

If needed, I can make the PR on merged master if this change seems appropriate :) Once again, thanks!

BrockReece commented 7 years ago

Hi @abruere,

Ooo, your right. I will address that tomorrow. It has now been merged into master and the major version has been bumped on NPM

Thanks for this Brock

Elhebert commented 7 years ago

@BrockReece @abruere

Sorry for the typo, mb 😆 I wasn't working with vue-quill anymore, and kind forgot about it. (left the company where I was using it ^^)

Actually, instead of the whole function I did wrote for setting up default values, you can use Object.assign (https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Object/assign).

Like this:

this.config = Object.assign({}, this.defaultConfig, this.config)

The line become:

this.editor = new Quill(this.$refs.quill, Object.assign({}, this.defaultConfig, this.config))
abruere commented 7 years ago

@Elhebert, I thought about Object.assign too, but it does not implement deep cloning, and could overwrite nested object options in quill config, for instance in some modules such as Quill history (docs). But I may be overstating here.

Thanks for the merge @BrockReece!

Cheers Andy

BrockReece commented 7 years ago

You are right @abruere, we need a deep merge.

I might just include the lodash defaults deep module. It should be pretty lightweight and way more reliable than anything I could write quickly.

https://www.npmjs.com/package/lodash.defaultsdeep

BrockReece commented 7 years ago

Cool so now V1.0.2 uses just the defaultsDeep.

Thanks for both of your input.