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

API for React Wrapper #49

Closed
James-E-Adams opened this issue Mar 24, 2018 · 6 comments
Closed

API for React Wrapper #49

James-E-Adams opened this issue Mar 24, 2018 · 6 comments

Comments

@James-E-Adams
Copy link

James-E-Adams commented Mar 24, 2018

I've got this to work so far, only supporting the highlight api.
Also this is based on writing it externally, but I'm open to ideas on how it should be bundled. It could either be a separate package, or exported from this one. i.e any of:

import {DriverReactWrapper as Driver} from 'driver.js'
import Driver from 'driver.js/react'
import Driver from 'driver.js.react'

For the most simple use case, I was thinking:

<Driver options={options}> 
    <ChildYouWishToFocus/>
</Driver>

This works fine- I've set up a create-react-app to test it, with below replacing the App.js.

import React, { Component } from "react";
import ReactDOM from "react-dom";
import logo from "./logo.svg";
import "./App.css";
import Driver from "driver.js";
import "driver.js/dist/driver.min.css";

class DriverWrapper extends Component {
  componentDidMount() {
    const id = ReactDOM.findDOMNode(this.childElement).id;
    this.driver = new Driver(this.props.options);
    this.driver.highlight(`#${id}`);
  }
  render() {
    const { children } = this.props;
    const childrenWithProps = React.Children.map(children, child =>
      React.cloneElement(child, { ref: el => (this.childElement = el) })
    );
    return childrenWithProps;
  }
}
class Child extends Component {
  render() {
    return <div id="cheese">Potato</div>;
  }
}

class App extends Component {
  render() {
    return (
      <DriverWrapper options={{ allowClose: false }}>
        <Child />
      </DriverWrapper>
    );
  }
}

export default App;

It would also be good if the driver.js supported something like driver.highlight(element), so you could pass in a dom element as an argument instead of a selector. I don't think it would be very hard to achieve with how the code is currently.

@James-E-Adams
Copy link
Author

Probably want to have a single driver instance for the app/page.

Potential solution could be to create a HOC called withDriver, and wrap a component high up in the tree, which will pass down a driver instance as a prop.

withDriver(Root)

Then, when you use the DriverWrapper, you pass a reference to the driver instance which can be passed through the props down the tree.

@kamranahmedse
Copy link
Owner

kamranahmedse commented Mar 24, 2018

Hey, while this looks good, I was thinking about a little different implementation to keep it simple and more generic for any use-case. We export a single component with which user provides all the options and then we subscribe to the lifecycle hooks of the component and manipulate the driver:

Below is the example to give you a better idea

import Driver from 'react-driver';

export class App extends React.Component {
  render: function () {
    return (
      <div className="app">
          <Driver
             ref="driver"
             start={true} // boolean for when you want to start it
             steps={[arrayOfSteps]} // this would be the array of step definitions
             highlight={element} // or you can pass this to highlight single element
             animate={true}
             opacity={0.75}
             ...
        />
        <YourComponents .../>
      </div>
    );
  }
}

@James-E-Adams
Copy link
Author

Hmmm. How would you envision accessing the elements for the highlight/steps? For the highlight, It may be better to pass a reference to the driver down the tree to wherever you want to highlight - or even something like highlight: (element) => this.driver.highlight(element). We could even make it smarter and allow the wrapper's highlight to accept a component as the argument, and use ReactDOM.findDomNode() to get a reference to the DOM element.

Additionally, I'm not sure how you would come up with an array of steps here without deeply knowing how the tree looks further into <YourComponents...> which kind of breaks the idea of encapsulation, unless I'm misunderstanding your suggestion.

BTW string refs are considered legacy, https://reactjs.org/docs/refs-and-the-dom.html#legacy-api-string-refs, so I'd avoid using them.

@kamranahmedse
Copy link
Owner

kamranahmedse commented Mar 24, 2018

How would you envision accessing the elements for the highlight/steps?

We don't need to access them in the wrapper, Driver would do that for us; you just need to pass the query selectors. I was thinking that the wrapper won't have anything reactish, it will just be a super simple single component using Driver behind the scenes the same way that we use it normally; except instead of initializing the Driver directly etc, we will do it through the wrapper. The component will just expose the options as attributes and proxy them over to the instance of Driver that it will create internally.

What do you think?

@James-E-Adams
Copy link
Author

Using the query selectors isn't a very react-y way of doing this - But I'm still not sure on the best way to do steps without it. It's hard because the very concept of steps breaks encapsulation in any case. I might write up a quick wrapper that only supports highlight for now, and you can see what you think. Will post it here when I'm done.

@James-E-Adams
Copy link
Author

https://github.com/James-E-Adams/react-driver.js

I still need to solve an issue with parcel/how I'm exporting the module though...

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

2 participants