antoniandre / wave-ui

A UI framework for Vue.js 3 (and 2) with only the bright side. ☀️
https://antoniandre.github.io/wave-ui
MIT License
544 stars 39 forks source link

[core.js] Tweak injectPresets to support mixin props and not error out #121

Closed DerrikMilligan closed 11 months ago

DerrikMilligan commented 11 months ago

Currently injectPresets errors out when you pass a prop that doesn't exist without any helpful messages. This adds a console.warn in those scenarios and continues execution.

Also, this adds the ability to inject props that are part of mixins for a component by copying the prop to the parent component for the injection.

DerrikMilligan commented 11 months ago

Sorry @antoniandre I don't have a nice codepen to show this off or explain it better.

In essence the use case for this would be allowing injecting a mixin prop on a component with that mixin. Like maybe for the w-menu someone wants to inject the preset for appendTo so that it always mounts to the activator instead of to the w-app.

I don't think this implementation would change any current behavior for overriding props and just expands it for the additional ones.

antoniandre commented 11 months ago

Thanks @DerrikMilligan, but I need to be able to test this piece of code before I release it. So what do you have in mind with this piece of code, what would you have outside of Wave UI? Once you give me a concrete use-case, I will merge, rewrite a little and publish

DerrikMilligan commented 11 months ago

Okay I'll do my best to explain it in a Codepen.

Currently we can have a section like this when registering WaveUI (See on Codepen):

app.use(WaveUI, {
  presets: {
    'w-button': { color: 'red' },
  }
})

But if we have a prop that doesn't exist on the component such as this (Uncomment line 9 in Codepen):

app.use(WaveUI, {
  presets: {
    'w-button': { badProp: 'whoops' },
  }
})

You'll get an error like this in the console: image

Which breaks all the WaveUI components on the page. The buttons in the codepen end up looking like this: image

This is what I attempted to address with this block of code in the injection script:

    // If we don't have the prop output a warning and continue
    if (component.props.hasOwnProperty(preset) === false) {
      console.warn(`Attempting to set preset on a prop that doesn't exist! Component: ${component.name} Preset: ${preset}`)
      continue
    }

By having a continue and a console.warn (We don't have to warn, I just thought it was convenient). We prevent breaking the rest of the page when the prop doesn't exist.

The other piece I have added allows for setting a mixin prop. My current use case for this would be setting a preset for the w-menu to default appendTo to be the string activator. The app I'm using Wave is a legacy codebase that I'm working to utilize WaveUI in. And there's some conflicting styles and some issues I'm working to overcome. Having the w-menu appending to the root w-app body element is causing me some issues. I can utilize appendTo="activator" on the individual element to solve this, but would like it to be a preset for now that I can remove later when this isn't an issue anymore.

Currently the way that injectPresets works is only overwriting props visible on the main component. It doesn't take into account mixin props which we can in actuality set on the component when using them. You uncomment line 11 in Codepen to try this. The w-menu component can definitely take the prop top from the mixin but this will throw the error.

That's what the other bit of code attempts to do. It checks to see if a component has mixins and if those mixins have the prop that we're looking to set a preset for. If it does then it assigns it as a prop on the actual element (which to my understanding is how a mixin works anyways). This then allows for setting presets for everything.

This is the code block that's checking for mixins:

    // Check to see if the component is missing the prop and we have mixins
    if (component.props.hasOwnProperty(preset) === false && Array.isArray(component.mixins) && component.mixins.length > 0)
      // Iterate over all the mixins
      for (const mixin of component.mixins)
        // Check to see if the mixin props contains the prop we want to set a preset for
        if (mixin.props && mixin.props.hasOwnProperty(preset))
          // Copy the mixin prop to the component prop and then let the original overwriting code change this to the new preset value below
          component.props[preset] = mixin.props[preset]

Hopefully that explains more what this is attempting to solve and my use case. I think that it just gives additional functionality without breaking anything as it currently works. And prevents the fatal failure when trying to set presets for invalid props.

If something doesn't make sense let me know and I can try and explain more.

antoniandre commented 11 months ago

Thanks for the details @DerrikMilligan! Now I understand about the mixins part - the rest I already agreed. I will review the code integration soon!

antoniandre commented 11 months ago

Released in version 3.5.2 :) 🚀