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

New API #132

Merged
merged 50 commits into from
Dec 14, 2015
Merged

New API #132

merged 50 commits into from
Dec 14, 2015

Conversation

gaearon
Copy link
Contributor

@gaearon gaearon commented Sep 24, 2015

This is a work in progress in extracting LogMonitor into a separate project and simplifying the API.

The todomvc example is currently broken, but counter example works.
A few notable changes:

  • <LogMonitor> is gone. It is now in a separate repo.
  • <DevTools> is gone. You are expected to use monitors directly and pass their arbitrary props and store={store} to them. The monitors are now expected to wrap their root component like this:
import { connectMonitor } from 'redux-devtools';

class MyMonitor extends Component {
  /* business as usual */
}

export default connectMonitor(MyMonitor);

This lets the consumer use them directly:

export default class Root extends Component {
  render() {
    const { store } = this.props;
    return (
      <div>
        <Provider store={store}>
          <CounterApp />
        </Provider>
        <Dock position='right' isVisible dimMode='none'>
          <LogMonitor store={store} />
        </Dock>
      </div>
    );
  }
}

Please hold off updating custom monitors until we settle down on the API.

  • <DebugPanel> is no more. I want the recommended solution to support resizing, moving and collapsing the panel. React Dock looks promising, but I'm not sure how to hook it up to persist the state without hardcoding this into the devTools logic. Ideally “monitor decorators” (one of which could be based on react-dock) should be able to hold their state in Redux devTools store independently. I'm not sure how to handle this yet, so suggestions are welcome.

@gaearon
Copy link
Contributor Author

gaearon commented Sep 25, 2015

Any feedback on this from the monitor authors?

@romseguy
Copy link
Collaborator

Looks fine and straight to the point. I'm on the verge of implementing an additional tree chart displaying the succession of actions leading to current state. It would allow to use commit, toggle, and rollback features by clicking on the actions nodes:

  • left click: toggle the action
  • right click: context menu with "rollback to this node", and "commit from this node".

Since the dev tools store would be shared between the two charts, they should be kept in sync.

This would get us the same features we have with the LogMonitor and allow for further improvement such as allowing the user to pick and choose between multiple action paths, if we are to index stagedActions to keep track of the parent actions and have a hierarchy of actions (makes sense?).

@gaearon
Copy link
Contributor Author

gaearon commented Sep 27, 2015

Oh, this is interesting. I'm probably going to push up another change here related to monitorState. I want monitors to (optionally) export a reducer to handle it—that would solve issues like #122 and give monitors more flexibility.

@gaearon
Copy link
Contributor Author

gaearon commented Sep 27, 2015

Okay, so I pushed this update which should let monitors be more powerful and potentially composable: 4ac31f9

The idea is that instead of setMonitorState(), the monitors now receive monitorActions which monitors themselves may choose by passing a parameter to connectMonitor(). The monitors may also specify their own reducer. When it exists, pass this reducer to devTools() as the first parameter, and it will manage monitorState.

Here is an example of how LogMonitor now manages its own visibility: gaearon/redux-devtools-log-monitor@05312f0. This should fix bugs like #122.

@gaearon
Copy link
Contributor Author

gaearon commented Oct 17, 2015

Fixed in gaearon/redux-devtools-dock-monitor@ecc67af, this will get into the next release.

@IwanKaramazow
Copy link

Glad I could help :)

@PeterKow
Copy link

Works great, thanks!
If someone is interested in upgrade commit check this out -> PeterKow/redux-react-babel-webpack@eff5418

@SimenB
Copy link

SimenB commented Oct 18, 2015

Not sure if I should report it here or in the redux-devtools-dock-monitor repo, but here goes 😄

This line https://github.com/gaearon/redux-devtools-dock-monitor/blob/ecc67af0e3f973d0bd915453253e6b1f88c1407c/src/DockMonitor.js#L51 stop all ctrl-shortcuts working, including ctrl+r to refresh, which is annoying. Could you only prevent default if it's actually handled in one of the cases?

EDIT: Created a PR for it. Might not be a problem for Mac-users, which is probably why it's not reported before. It seems everybody uses Macs these days 😆

@gaearon
Copy link
Contributor Author

gaearon commented Oct 18, 2015

Thanks for reporting, indeed that repo is the correct place.

@chentsulin
Copy link
Contributor

@gaearon Will 3.0 have a helper for create DevTools on new window, or just do something like this?

// containers/Devtools
import React from 'react';
import { createDevTools } from 'redux-devtools';
import LogMonitor from 'redux-devtools-log-monitor';

export default createDevTools(
  <LogMonitor />
);

// utils/createDevToolsWindow.js
import React from 'react';
import { render } from 'react-dom';
import { Provider } from 'react-redux';
import DevTools from '../containers/DevTools';

/**
 * Puts Redux DevTools into a separate window.
 * Based on https://gist.github.com/tlrobinson/1e63d15d3e5f33410ef7#gistcomment-1560218.
 */
export default function createDevToolsWindow(store) {
  // Window name.
  const name = 'Redux DevTools';

  // Give it a name so it reuses the same window.
  const win = window.open(
    null,
    name,
    'menubar=no,location=no,resizable=yes,scrollbars=no,status=no'
  );

  // Reload in case it's reusing the same window with the old content.
  win.location.reload();

  // Set visible Window title.
  win.document.title = name;

  // Wait a little bit for it to reload, then render.
  setTimeout(() => render(
    <Provider store={store}>
      <DevTools />
    </Provider>,
    win.document.body.appendChild(document.createElement('div'))
  ), 100);
}

@chentsulin
Copy link
Contributor

Or maybe a SideEffect Component <DevToolsWindow>?

@rileylark rileylark mentioned this pull request Oct 28, 2015
@kumar303
Copy link

I just tried hooking up dev tools for the first time with the latest beta (3) and everything is pretty much working fine. The only thing for me is the key bindings don't do anything. No errors, just no hiding when I press H or h. It didn't work in any browser I tried so maybe I hooked it up wrong? This was my patch to add the dev tools.

@SimenB
Copy link

SimenB commented Nov 19, 2015

It's ctrl-h and ctrl-q

@kumar303
Copy link

Oh, duh, thanks! That works in Chrome. I still get the ReferenceError in FF but it looks like that fix hasn't been released yet.

@Hobby-Student
Copy link

Hey out there,

hope this one is the right topic to report. I'm from Germany and so the "@" is used by "AltGr+q" (the same as "Ctrl+Alt+q"), which is prevented by the DockMonitor... (using FF for testing)

edit: forgot to mention... using e.g. changePositionKey='W' results in the same behavior... no "@"

This is the original code:

DockMonitor.prototype.handleKeyDown = function handleKeyDown(e) {
    if (!e.ctrlKey) {
      return;
    }
    e.preventDefault();

    var key = e.keyCode || e.which;
    var char = String.fromCharCode(key);
    switch (char.toUpperCase()) {
      case this.props.toggleVisibilityKey.toUpperCase():
        this.props.dispatch(_actions.toggleVisibility());
        break;
      case this.props.changePositionKey.toUpperCase():
        this.props.dispatch(_actions.changePosition());
        break;
      default:
        break;
    }
  };

which I modified a bit:

DockMonitor.prototype.handleKeyDown = function handleKeyDown(e) {
    if (!e.ctrlKey || e.altKey) {                               //<--- this one
      return;
    }
    e.preventDefault();

    var key = e.keyCode || e.which;
    var char = String.fromCharCode(key);
    switch (char.toUpperCase()) {
      case this.props.toggleVisibilityKey.toUpperCase():
        this.props.dispatch(_actions.toggleVisibility());
        break;
      case this.props.changePositionKey.toUpperCase():
        this.props.dispatch(_actions.changePosition());
        break;
      default:
        break;
    }
  };

@SimenB
Copy link

SimenB commented Nov 23, 2015

@Hobby-Student
Copy link

@SimenB thanks... and sry for hijacking this one ;)

@gaearon gaearon changed the title (WIP) Extract LogMonitor and change the API New API Dec 14, 2015
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.