Skip to content

Conversation

@thcolin
Copy link

@thcolin thcolin commented Jul 28, 2017

Hi !

My friend @soyuka told me about your library for my current React project, and he show me your ongoing pull request to improve React support without interfering too much with current implementation (Some initial work on React support #8).

So I work a bit on it and here's my suggestion :

  • Implement inspectElement and transformElement functions and make them configurable (as well as mergeStyle)
  • By default HyperList will use its own methods (HyperList.inspectElement, HyperList.transformElement, HyperList.mergeStyle) which expect a DOM element as first args
  • Create an helper for specific framework (currently React)
    • This helper component will use its own inspectElement, transformElement and mergeStyle because of Virtual DOM usage

This solution should allow you to support various not DOM-friendly frameworks in the long term.

If you're okay with my changes, I'll make another commit to remove react-example.html and react specific dev-dependecies and then create a hyperlist-react npm module to bundle HyperListReact React component and usage documentation. It would be helpful to have a link to futur hyperlist-react repo on README project too.

HyperListReact component would look like this :

class HyperListReact extends React.Component {
  render() {
    return React.createElement('div', this.state.element,
      this.state.fragment
    );
  }

  constructor(props) {
    super(props);

    this.handleRef = this.handleRef.bind(this);

    this.state = {
      element: {
        ref: this.handleRef
      },
      scroller: {
        key: 'scroller',
        style: {
          position: 'relative'
        }
      },
      fragment: [],
      node: {}
    };
  }

  handleRef(node) {
    this.setState({ node })
  }

  componentDidMount() {
    const {
      height = window.innerHeight,
      itemHeight = 50,
      total = 0,
      reverse = false,
      generate = () => {}
    } = this.props;

    const config = {
      height,
      itemHeight,
      total,
      generate,
      scroller: React.createElement('div', this.state.scroller),
      useFragment: false,
      overrideScrollPosition: () => this.state.node.scrollTop || 0,
      inspectElement: (element, key) => element.props[key === 'class' ? 'className' : key] || null,
      transformElement: (element, values) => {
        if (values.class) {
          values.className = values.class
          delete values.class
        }

        return React.cloneElement(element, values)
      },
      mergeStyle: (element, style, forceClone) => {
        if (forceClone) {
          return React.cloneElement(element, { style })
        }

        for (let i in style) {
          if (element.props.style[i] !== style[i]) {
            element.props.style[i] = style[i]
          }
        }

        return element
      },
      applyPatch: (element, fragment) => {
        this.setState({
          element: Object.assign({}, this.state.element, {
            style: Object.assign({}, this.state.element.style, element.props.style)
          }),
          fragment
        });
      },
    };

    this.list = HyperList.create(this, config);

    // Bind to the resize event, and since you should only ever have one
    // handler bound to this, we pave over whatever you had set before.
    window.onresize = e => {
      config.height = window.innerHeight;
      this.list.refresh(this, config);
    };
  }

  componentWillUnmount() {
    window.onresize = null;
    this.list.destroy();
  }
}

HyperListReact.defaultProps = {
  style: {}
}

With same usage as #8 (see examples/react-example.html for full example) :

class MyComponent extends React.Component {
  render() {
    return React.createElement(HyperListReact, {
      itemHeight: 30,
      total: 100000,
      generate: this.generate
    })
  }
  generate(row) {
    return React.createElement('div', {}, 'Item ' + row);
  }
}

}

const diff = lastRepaint ? scrollTop - lastRepaint : 0
if (!lastRepaint || diff < 0 || diff > this._averageHeight) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

const diff = lastRepaint ? scrollTop - lastRepaint : 0 
if (!lastRepaint || diff < 0 || diff > this._averageHeight) {

is correct

Copy link
Author

Choose a reason for hiding this comment

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

Taken from 6ef597a7af3970f892f8457d5ac5292fdabc1589, but yes, it shouldn't be in the commit

refresh (element, userProvidedConfig) {
Object.assign(this._config, defaultConfig, userProvidedConfig)

if (!element || element.nodeType !== 1) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why typeof element.render !== 'function'?

Copy link
Author

Choose a reason for hiding this comment

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

I suppose any element of a VirtualDOM has a render function, cf. Matt-Esch/virtual-dom

.gitignore Outdated
node_modules
coverage
dist
package-lock.json
Copy link
Collaborator

Choose a reason for hiding this comment

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

package-lock should be commited

@soyuka
Copy link
Collaborator

soyuka commented Aug 7, 2017

wdyt @tbranyen ?

@tbranyen
Copy link
Owner

Looks good to me. I think I may have just caused a conflict by merging. Can we get this rebased to master and I'll give it a whirl in my app!

@soyuka
Copy link
Collaborator

soyuka commented Sep 20, 2017

@tbranyen I've worked a bit more on this and rebase will come in a couple of days ;)

@thcolin
Copy link
Author

thcolin commented Oct 11, 2017

Sorry, I'll won't be able to complete this PR, I tried last week without any luck, so I made my own in React way, sorry 😔 I deprecated my repo thcolin/hyperlist-react, but anyone who want to start from where I was can fork it of course ! I tried tounpublish the library on npm but they changed their policy so I could only deprecate it but see :

If another member of the community wishes to publish a package with the same name as a security placeholder, they’ll need to contact support@npmjs.com. npm will determine whether to grant this request. (Generally, we will.)

Sorry for any false hope 😔

@thcolin thcolin closed this Oct 11, 2017
@tbranyen tbranyen reopened this Nov 2, 2017
@tbranyen
Copy link
Owner

tbranyen commented Nov 2, 2017

@thcolin I'm going to reopen this since it's interesting work and maybe someone else can pick it up. It's open source after-all.

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