Added shift toggle #47

khankuan · 2016-04-03T07:42:13Z

No description provided.

gaearon · 2016-04-03T10:29:31Z

src/LogMonitor.js

@@ -72,6 +72,10 @@ export default class LogMonitor extends Component {

  shouldComponentUpdate = shouldPureComponentUpdate;

+  state = {


Can we use monitorState and reducers for that? We already have a reducer tracking current scroll top, for example. Local state will get erased on reload, but with persistState(), monitorState will not.

Oh you're totally right. Can't believe i didn't notice the reducer.js and action.js..

gaearon · 2016-04-03T10:29:58Z

Do you mind providing a GIF showing how it works?

khankuan · 2016-04-03T19:16:58Z

Hmm, i was using redux-devtools-extension and realised that its not using the default instrument. In that case, the reducer is not in used. Any thoughts about it?

zalmoxisus · 2016-04-04T11:06:16Z

We use default instrument in redux-devtools-extension, but the monitors are outside the app side (and the instrument enhancer). So, yes the monitors reducers have no effect there, but will be fixed in zalmoxisus/redux-devtools-extension#87.

zalmoxisus · 2016-04-12T08:53:03Z

src/LogMonitor.js

@@ -160,7 +161,7 @@ export default class LogMonitor extends Component {

  handleToggleConsecutiveAction(id) {
    const { consecutiveToggleStartId } = this.props.monitorState;
-    if (consecutiveToggleStartId !== null) {
+    if (consecutiveToggleStartId > -1) {


It works only first time, while consecutiveToggleStartId is undefined and the condition is not satisfied. Next time consecutiveToggleStartId will be set to null and null > -1. So it will be always interpreted as a starting point, and there won't be possible to set another start point than null anymore.

Why not just to have if (consecutiveToggleStartId)? It will also prevent toggling the first INIT state. I tried and this fixes the issue.

zalmoxisus · 2016-10-21T10:48:42Z

@khankuan, I'd like to merge this. If you could find time to fix the conflicts, I'll review it and will add support for monitor reducers in the extension.

Also, would be nice to identify actions to be toggled in some other way. Maybe an interrupted or flashing line?

khankuan · 2016-10-22T05:05:55Z

@zalmoxisus cool :) I've fixed the merge conflicts. However, I would like to get some help on the best way to run and test the monitor visually.

zalmoxisus · 2016-10-22T08:49:12Z

I guess the easier way would be to have an example here. For now you can:

Run npm link here.
Clone redux-devtools repo and npm install.
Chose an example, for example cd examples/todomvc, and do npm install.
Remove redux-devtools-log-monitor from there (rm -rf node_modules/redux-devtools-log-monitor).
Run npm link redux-devtools-log-monitor.

khankuan · 2016-10-31T00:38:17Z

Hey, i've updated the branch and tested. Used opacity for selected state instead. I'm interested in the flashing line, would it required redux-devtools-theme to take in css?

zalmoxisus · 2016-10-31T14:36:24Z

@khankuan, css animation with inline styles is a pain. I think we could just keep your implementation with opacity, marking that it's still not applied. Maybe you have any other ideas of showing that the action is still in process?

khankuan · 2016-11-01T04:34:18Z

Here's how it feels like now:

zalmoxisus · 2016-11-01T08:34:58Z

@khankuan, looks great, thanks. There's still an issue here to be addressed.

khankuan · 2016-11-02T02:28:47Z

Hmm I dont see it. Is the review in draft mode?

zalmoxisus · 2016-11-02T08:05:37Z

@khankuan, weird :)

The reducer works ok, but, when having other actions dispatched meantime, the selection is lost.

Returning null for other than START_CONSECUTIVE_TOGGLE actions here, leads to unexpected behaviours. For example, when scrolling, initialScrollTop is dispatched and the selection gets dismissed. Also, I guess wile selecting some app's actions can dispatched and we don't want to lose the selection.

I'd suggest to dismiss the selection only when regular COMMIT is dispatched. However, we cannot handle this case here as for the extension we call monitors reducers only for its actions. So, a solution is when dispatching COMMIT, to dispatch also { type: START_CONSECUTIVE_TOGGLE, id: null }.

Another case is when the selected action got removed (due to commiting, reseting, reaching maxAge...), for that we shouldn't take it into consideration just by checking if it's present when making the second shift toggle.

khankuan · 2016-11-02T16:37:50Z

Ah interesting! Do you think we can use a method similar to

redux-devtools-log-monitor/src/LogMonitor.js

Line 112 in 9d87092

node.addEventListener('scroll', this.updateScrollTop);

, listen to keyup events.

zalmoxisus · 2016-11-02T18:57:58Z

@khankuan it would solve just a part of the problem. I think we should just return the previous value instead of null so it will not get dismissed while scrolling (initialScrollTop is dispatched).

khankuan · 2016-11-03T07:12:07Z

Oh yes definitely meant that. But more importantly, would the keyup event method be the best way?

zalmoxisus · 2016-11-03T09:06:01Z

@khankuan, no, listening to keyup events wouldn't be a solution, at least because actions can be removed due to reaching maxAge. Better and simpler is just to dismiss it (set to null) when handleToggleAction is called and to ignore consecutiveToggleStartId when the action with that id isn't present anymore.

… valid to dispatch setActionsActive

khankuan · 2016-11-28T05:13:40Z

@zalmoxisus sorry for the delay, updated accordingly here: 11e698c

zalmoxisus · 2016-11-29T19:17:51Z

@khankuan, no worries, thanks a lot for the changes. I see 2 issues here:

Seems like you're not passing state to startConsecutiveToggle, but state.consecutiveToggleStartId.
Handling the case when the action is not present anymore works great. Apart from that, shifting works only first time, then the first action will be always the same, and selecting a new start id will result in untoggling the interval.

khankuan · 2016-11-30T04:32:56Z

Hmm, for 1, would there be any consequence of preserving via store state? For 2, I'm trying to understand it a little more. I was expecting the ux flow to be:

holds shift key
click on starting action
action gets "selected" to be the start
on the next user click on an action, if shift key is active, then trigger the consecutive toggle.

zalmoxisus · 2016-11-30T09:28:26Z

Hmm, for 1, would there be any consequence of preserving via store state?

It just throws as you try to access state.consecutiveToggleStartId.consecutiveToggleStartId.

on the next user click on an action, if shift key is active, then trigger the consecutive toggle.

First time it will work as expected, then you'll not be able to select the first action anymore, as it is already selected (consecutiveToggleStartId is set and cannot be changed).

zalmoxisus · 2017-01-03T20:19:59Z

Hey @khankuan,

I'm merging this, will add the missing part in another PR. Thanks a lot for the contribution!

…ft toggling Related to #47 (comment) nt-263669403

khankuan · 2017-01-04T01:48:39Z

@zalmoxisus thanks for maintaining this project :)

khankuan added 2 commits April 3, 2016 00:40

Added shift toggle

1e72568

Added selected to entry

d6159e6

khankuan mentioned this pull request Apr 3, 2016

Consecutive toggle reduxjs/redux-devtools#248

Merged

gaearon reviewed Apr 3, 2016
View reviewed changes

khankuan added 2 commits April 3, 2016 09:42

Change to monitorState

5caec74

fix binding

552b04a

zalmoxisus reviewed Apr 12, 2016
View reviewed changes

Prevent init from being toggled

5b0c7bb

zalmoxisus mentioned this pull request Oct 21, 2016

[Suggestion] Highlight recent action in inspector when using slider zalmoxisus/redux-devtools-extension#234

Closed

khankuan added 2 commits October 21, 2016 21:51

merge master

077e5d7

update missing props

7f3bbbf

Fix selection to end + 1, change styling for selected

5c2b1f2

Retain start index instead of defaulting to null. Ensure action id is…

11e698c

… valid to dispatch setActionsActive

zalmoxisus changed the base branch from master to shift-toggle January 3, 2017 20:20

zalmoxisus merged commit fc216e0 into gaearon:shift-toggle Jan 3, 2017

zalmoxisus added a commit that referenced this pull request Jan 3, 2017

Fix accessing state.consecutiveToggleStartId and clear it after shi…

0e1cf42

…ft toggling Related to #47 (comment) nt-263669403

zalmoxisus mentioned this pull request Jan 3, 2017

Shift toggle #62

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added shift toggle #47

Added shift toggle #47

khankuan commented Apr 3, 2016

gaearon Apr 3, 2016

khankuan Apr 3, 2016

gaearon commented Apr 3, 2016

khankuan commented Apr 3, 2016

zalmoxisus commented Apr 4, 2016 •

edited

Loading

zalmoxisus Apr 12, 2016

zalmoxisus commented Oct 21, 2016 •

edited

Loading

khankuan commented Oct 22, 2016

zalmoxisus commented Oct 22, 2016

khankuan commented Oct 31, 2016

zalmoxisus commented Oct 31, 2016 •

edited

Loading

khankuan commented Nov 1, 2016

zalmoxisus commented Nov 1, 2016

khankuan commented Nov 2, 2016

zalmoxisus commented Nov 2, 2016

khankuan commented Nov 2, 2016

zalmoxisus commented Nov 2, 2016

khankuan commented Nov 3, 2016

zalmoxisus commented Nov 3, 2016 •

edited

Loading

khankuan commented Nov 28, 2016

zalmoxisus commented Nov 29, 2016

khankuan commented Nov 30, 2016

zalmoxisus commented Nov 30, 2016 •

edited

Loading

zalmoxisus commented Jan 3, 2017

khankuan commented Jan 4, 2017

		@@ -72,6 +72,10 @@ export default class LogMonitor extends Component {

		shouldComponentUpdate = shouldPureComponentUpdate;

		state = {

Added shift toggle #47

Added shift toggle #47

Conversation

khankuan commented Apr 3, 2016

gaearon Apr 3, 2016

Choose a reason for hiding this comment

khankuan Apr 3, 2016

Choose a reason for hiding this comment

gaearon commented Apr 3, 2016

khankuan commented Apr 3, 2016

zalmoxisus commented Apr 4, 2016 • edited Loading

zalmoxisus Apr 12, 2016

Choose a reason for hiding this comment

zalmoxisus commented Oct 21, 2016 • edited Loading

khankuan commented Oct 22, 2016

zalmoxisus commented Oct 22, 2016

khankuan commented Oct 31, 2016

zalmoxisus commented Oct 31, 2016 • edited Loading

khankuan commented Nov 1, 2016

zalmoxisus commented Nov 1, 2016

khankuan commented Nov 2, 2016

zalmoxisus commented Nov 2, 2016

khankuan commented Nov 2, 2016

zalmoxisus commented Nov 2, 2016

khankuan commented Nov 3, 2016

zalmoxisus commented Nov 3, 2016 • edited Loading

khankuan commented Nov 28, 2016

zalmoxisus commented Nov 29, 2016

khankuan commented Nov 30, 2016

zalmoxisus commented Nov 30, 2016 • edited Loading

zalmoxisus commented Jan 3, 2017

khankuan commented Jan 4, 2017

zalmoxisus commented Apr 4, 2016 •

edited

Loading

zalmoxisus commented Oct 21, 2016 •

edited

Loading

zalmoxisus commented Oct 31, 2016 •

edited

Loading

zalmoxisus commented Nov 3, 2016 •

edited

Loading

zalmoxisus commented Nov 30, 2016 •

edited

Loading