whetstone / redux-devtools-diff-monitor

https://www.npmjs.com/redux-devtools-diff-monitor
176 stars 14 forks source link

Pass style to DiffMonitor #5

Closed sergey-lapin closed 8 years ago

sergey-lapin commented 9 years ago

Now DiffMonitor has position fixed, so it overflow user content. Is it possible to set it's style passing props?

gtfiorentino commented 9 years ago

@lapanoid: you're thinking just be able to pass in an arbitrary style object as a prop? E.g. <DevTools store={store} monitor={DiffMonitor} style={{color: 'red'}} />?

Or is it better to provide specific props that hook into specific styles, e.g. <DevTools store={store} monitor={DiffMonitor} color='red' /> and have the monitor handle applying that internally?

The first approach is obviously more flexible, but the second feels more declarative for specific attributes we might want to make customizable. In part I think it depends how the tool is used and if there are any common customization points.

Thoughts?

gtfiorentino commented 9 years ago

FYI I tossed up the first option (pass one style prop) to a branch called abritrary-styles to see how that feels. Note that it's set up to style the top level monitor component only (at least for now).

Is your concern specifically about the positioning? If you think there's a better way to handle the positioning, I'd be interested to hear that as well.

sergey-lapin commented 9 years ago

@gtfiorentino I actually work on some solution of monitors positioning here which was driven by this discussion. I think there is no way to handle position clever if you don't know positions and sizes of other monitors and user content. That leads to the idea that every monitor should be styled directly from some parent. Of course, some styles should not be changed, so I guess there are two ways: you can just export your styles, so user can change it somehow and pass it to DiffMonitor, pretty simple, or you can specify your component that user be able to set only width, height, position, etc. I prefer the first one as it is more transparent and strict way

sergey-lapin commented 9 years ago

Now my biggest concern is position:fixed as there are literally no way around to fix this outside of DiffMonitor.

gtfiorentino commented 9 years ago

I'd like DiffMonitor to be usable both standalone and as a part of something like your monitor dock. In either scenario, DebugPanel should handle the positioning, so I think we can take the positioning out of DiffMonitor.

sergey-lapin commented 9 years ago

Thanks! Looking forward new npm version.

iktl commented 8 years ago

I've done some housekeeping and bumped this to 5.0.2 with some functional and performance updates, but don't have time to work on this feature. We would gladly accept a PR if there is interest in it, but otherwise I'm going to close this for the time being.