Adding JS mixin ability to PatternFly Elements

[kylebuch8]

We have a few components that are going to need to share functionality but those two components are completely unrelated: think pfe-tabs and pfe-scrollspy. There are two approaches that I can think of to accomplish this. We could create a wrapper component that would progressively enhance the children. Or we could explore mixins (which is the approach I would favor).

Use case

1. Clicking on a pfe-tab could update the hash in the url or open a specific pfe-tab based on the hash in the URL.
2. pfe-scrollspy could update the hash in the URL as you scroll to a specific section of the page.

Technical notes

I've seen a lot of people suggesting Object.assign for creating a mixin, but Justin Fagnani cleared it up for as to why this is a bad idea.

I think we should just use the simple pattern provided on MDN. It would allow us add any number of mixins to a class.

import { hashUpdater, anotherMixin, howAboutAnother } from "./mixins.js"

class PfeThing extends hashUpdater(anotherMixin(howAboutAnother(PFElement))) {
  constructor() {
    this.updateHash('scrollhere');
  }
}

Current issues

gulpfile.factory.js

We'd have to update line #161 in scripts/gulpfile.factory.js to accept any number of mixins.

/extends\s+.*\(?P[Ff][Ee][A-z0-9_$]*\)?\s+{/g,

This was my first attempt to update the regular expression, but it doesn't handle the case of multiple mixins.

rollup.config.factory.js

If I were to add a mixins directory to ./elements/pfelement, I don't think it would get picked up by Gulp or Rollup to move it to the _temp directory and create a ES6 and ES5 version of the file. We'd have to make an adjustment here.

Overall

I think this would be great for us to explore. We might be able to reduce the size of our PFElement base class by moving pieces of it to mixins. Also, it would help us not add more markup to pages by creating a bunch of wrapper components.

[castastrophe]

Love the mixins approach! 👍

[mwcz]

I think we can accomplish the same goal without this syntax, which I find pretty convoluted.

import { hashUpdater } from "./mixins.js"

class PfeThing extends hashUpdater(PFElement) {
  constructor() {
    this.updateHash('scrollhere');
  }
}

I'd be move in favor of a simpler, module-based approach like this:

import { hash } from "./common/hash.js"

class PfeThing extends PFElement {
  constructor() {
    hash.updateHash('scrollhere');
  }
}

Easier to follow, right? Mixins are great when they hook themselves in to your class's existing structure (ie, the mixin calls itself). But in cases like this where you're manually calling upateHash, there's no advantage to baking the mixin functionality into this.

It's possible that a feature requirement will come along that takes full advantage of the mixin patten. My preference would be to go with the simpler module pattern until that need arises, and revisit mixins if it does.

[heyMP]

Giving this a bump to see what everyone's thoughts are this 1 year later :)

I agree with @mwcz 's critique of the mixin pattern being used for just sharing functionality, BUT, an advantage that mixins have is the ability to share public methods and properties between components.

For example, if we had pfe-toolbar and pfe-popover. We would want both of those elements to have the ability to be absolutely positioned over a target element. In addition we would want to surface some standard options associated with that functioanlity such as, offset and position.

<pfe-tooltip position="top" offset="3px"><pfe-tooltip>

<pfe-popover position="bottom" offset="5px"></pfe-popover>

So worst case scenario is that you just have to maintain two different API's that will be identical. Not really a gigantic deal but something to consider.

I've seen some libraries that went mixin crazy and I can't make sense of anything. But I can see a mixin being helpful in the above scenario. ¯_(ツ)_/¯

[castastrophe]

I think a potentially better approach to the similarities of tooltip and popover is the example of pfe-collapse. Collapse has no opinions about styles. It just defines the API and functionality; then components like accordion can extend it and apply syntax sugar.

[bennypowers]

We should do this with reactive controllers

candidates:

• pfe-tabs
• pfe-accordion
• pfe-jump-links
• pfe-select

and others