Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow required elements to be passed as options (React) #488

Closed
wants to merge 5 commits into from

Conversation

smartmike
Copy link

@smartmike smartmike commented Nov 11, 2016

This is useful for use in React (Or any other framework relying on a DOM diff).

Problem

You want a slider in React with changing content, you render your components, and initialise the slider onComponentDidMount. The props change, the slides change, one gets removed, added, just completely different, React loses the reference to the children and throws an error. Something like The node to be removed is not a child of this node..

Solution

Don't manipulate the DOM from Flickity, pass in the containers you already render with React.

const Slider = React.createClass({
  componentDidMount () {
    const options = {
      viewport: this.viewportNode,
      slider: this.sliderNode
    }

    this.flickity = new Flickity(this.flickityNode, options)
  },

  componentWillUnmount () {
    this.flickity.destroy()
  },

  render () {
    return (
      <div ref={(ref) => (this.flickityNode = ref)}>
        <div className='flickity-viewport' ref={(ref) => (this.viewportNode = ref)}>
          <div className='flickity-slider' ref={(ref) => (this.sliderNode = ref)}>
            {this.props.children}
          </div>
        </div>
      </div>
    )
  }
})

@desandro
Copy link
Member

Thanks so much for this contribution. I'm not familiar with React, so I appreciate your expertise here.

After looking again at #381, I like how it does not separate options, but rather just one option, then expects the appropriate markup to be in the page. Do you have a preference?

@smartmike
Copy link
Author

smartmike commented Nov 14, 2016

@desandro I like the combined option on #381, I don't like however the way of finding the elements, I'd much rather be able to pass Flickity the actual elements I'm using... – but maybe that's just being picky.

@desandro
Copy link
Member

Ah, thanks! Could you elaborate? Would finding the elements be problematic? Maybe you don't like requiring to use classnames like flickity-viewport ?

@smartmike
Copy link
Author

Right, being able to pass the elements would help with using css modules for example where the classes get mangled, but that's really not a blocker, just a personal preference. :)

@KittyGiraudel
Copy link

Hey there, just thought I’d give my 2 cents here.

I think it’s a very nice addition to be able to pass the elements Flickity needs. I totally get why it’s not the default: we want it to be as easy as possible to use. One container, not three. That makes a lot of sense.

However, letting Flickity doing DOM manipulation (talking tree structure here, not attribute updates) makes it somehow hard to use (and possibly buggy as we can see here) with any library using a virtual DOM (React, Vue.js, Preact…).

The solution proposed here is both very elegant and backward-compatibly, not to mention simple. I really think it should be part of the library. If the lack of documentation is a problem, I’d be happy to PR the docs.

@desandro
Copy link
Member

@hugogiraudel Thanks for your input! I'm currently leaning towards #381, which skips over the options and assumes that flickity-viewport and flickity-slider are already in the DOM, with the proper classnames. I find it simpler for users as they will not require two options AND it matches up with CSS. What do you think?

@KittyGiraudel
Copy link

That might be a decent middleground, yes.

@KittyGiraudel
Copy link

Any idea when you’ll release one of these 2 PRs Dave? Can we help you with anything?

@desandro
Copy link
Member

No plans for release in the near future. This feature is a tricky one as it undermines several areas of Flickity. I'll get around to it when I'm ready to look at Flickity again.

@smartmike
Copy link
Author

Good news, as of React 16, and Portals this is no longer required.

Here's a quick untested sample:

import React from 'react'
import ReactDOM from 'react-dom'
import Flickity from 'flickity'

class Slider extends React.Component {
  static defaultProps = {
    options: {}
  }

  componentDidMount () {
    this.flickity = new Flickity(this.flickityNode, this.props.options)
  }

  renderSlider () {
    if (!this.flickityNode) {
      return null
    }

    const mountNode = this.flickityNode.querySelector('.flickity-slider')

    if (mountNode) {
      return ReactDOM.createPortal(this.props.children, mountNode)
    }
  }

  render () {
    return [
      <div key='slider-base' ref={node => this.flickityNode = node} />,
      this.renderSlider()
    ]
  }
}

And use it like:

<Slider options={{ autoPlay: true }}>
  <div>Slide 1</div>
  <div>Slide 2</div>
  <div>Slide 3</div>
  <div>Slide 4</div>
</Slider>

@smartmike smartmike closed this Oct 25, 2017
@desandro
Copy link
Member

@smartmike Thank you for following up!

@ryanwiemer
Copy link

Hey @smartmike I found this via a Google search as I am trying to get Flickity working with React. Did you ever put together a proof of concept with what you suggested with React 16? Or is there code anywhere, perhaps in Codepen, that shows a functional example?

Thanks,
Ryan

@smartmike
Copy link
Author

@ryanwiemer Sure – take a look here: https://codepen.io/smartmike/pen/dZBNVv

@hinok
Copy link
Contributor

hinok commented Aug 8, 2018

Dragging doesn't work since Flickity@2.1.x so one line is more required in refreshFlickity() from @smartmike example:

  refreshFlickity () {
    // NOTE: this could probably be smarter
    this.flickity.reloadCells()
    this.flickity.resize()
    this.flickity.updateDraggable() // <---- This is required to fix dragging for Flickity 2.1.x
  }

Demo: https://codepen.io/hinok/pen/djQboa

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants