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

Delegate to React the DOM manipulation #1

Closed
FezVrasta opened this issue Oct 18, 2016 · 5 comments
Closed

Delegate to React the DOM manipulation #1

FezVrasta opened this issue Oct 18, 2016 · 5 comments

Comments

@FezVrasta
Copy link
Member

Right now seems like you are letting Popper use the applyStyle modifier to directly edit the DOM and apply the needed styles.

I think it would be much better to disable applyStyle and use React to update the DOM.

Here there's an example:
https://gist.github.com/FezVrasta/6533adf4358a6927b48f7478706a5f23

@souporserious
Copy link
Collaborator

I looked into this and then realized we are already touching the DOM by creating a node to stick the Popper in https://github.com/souporserious/react-popper/blob/master/src/PopperManager.jsx#L65-L71 so I figured we didn't need to worry about it. Maybe I'm wrong in that thinking. Thoughts?

@FezVrasta
Copy link
Member Author

FezVrasta commented Oct 18, 2016

It would be less DOM that is touched directly. I think everything should be managed by React when possible. In this way it can make its optimizations.

@FezVrasta
Copy link
Member Author

FezVrasta commented Dec 30, 2016

This is how the Popper should be initialized:

this._popper = new Popper(this._referenceNode, popperNode, {
  placement,
  modifiers: {
    ...modifiers,
    applyStyle: { enabled: false },
    extractPosition: {
      enabled: true,
      order: 800,
      function: this._extractPosition,
    },
    arrow: { element: this._arrowNode }
  },
})

And this is the modifier that extracts the position:

_extractPosition = (data) => {
  const { gpuAcceleration } = this.props
  const {
    attributes: popperAttributes
  } = data
  
  const popperStyle = {
    position: data.offsets.popper.position,
  }
  
  if (gpuAcceleration) {
    popperStyle.left = 0
    popperStyle.top = 0
    popperStyle.transform = `translate3d(${Math.round(data.offsets.popper.left)}px, ${Math.round(data.offsets.popper.top)}px, 0)`
  } else {
    popperStyle.left = data.offsets.popper.left
    popperStyle.top = data.offsets.popper.top
  }
  
  this.setState({
    popperAttributes: data.attributes,
    popperStyle,
    arrowStyle: data.offsets.arrow,
  })
}

It would make sense to allow react-popper to render the Popper as adjacent element of its reference thus letting React do everything (no portal/render to body). Popper works quite well in this situation.

@victorias
Copy link

I started my own fork based on your example code where React manages the popper instance directly and just references Popper's configs here: https://github.com/victorias/react-popperjs . Would love some feedback!

@souporserious
Copy link
Collaborator

souporserious commented Feb 24, 2017

Styles are now applied through React 🎉

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

No branches or pull requests

3 participants