New redux action in editor#4457
Conversation
| addOrToggleDisabledBreakpoint: line => | ||
| dispatch(addOrToggleDisabledBreakpoint(line)), | ||
| traverseResults: (rev, editor) => dispatch(traverseResults(rev, editor)) | ||
| }; |
There was a problem hiding this comment.
Ooh, I like this style, but let's discuss adopting it in a separate PR.
I honk we can land his fix without I and see what style for actions is most used in the community. I would Perper something as explicit, but more terse. It might not exist :)
There was a problem hiding this comment.
I can roll back the style changes. Let's have the discussion here: #4454
There was a problem hiding this comment.
Just so this doesn't get lost, we can avoid the function entirely like this.
| }; | ||
|
|
||
| class Editor extends PureComponent<Props, State> { | ||
| props: Props; |
There was a problem hiding this comment.
I don think we need this
| toggleBreakpoint: number => void, | ||
| addOrToggleDisabledBreakpoint: number => void, | ||
| jumpToMappedLocation: any => void, | ||
| traverseResults: (boolean, Object) => void |
| } | ||
|
|
||
| if (conditionalPanelLine) { | ||
| return closeConditionalPanel(); |
There was a problem hiding this comment.
Hmm, I'd like to check elsewhere but I believe in functions with multiple actions we destructure for brevity
| }; | ||
| }; | ||
|
|
||
| const mapDispatchToProps = dispatch => { |
|
I rolled back the redux changes in this PR, so that it can be merged @jasonLaster @wldcordeiro |
codehag
left a comment
There was a problem hiding this comment.
Nice work so far. a couple of comments
|
|
||
| const direction = e.shiftKey ? "prev" : "next"; | ||
| traverseResults(e, ctx, query, direction, searchModifiers.toJS()); | ||
| this.props.traverseResults(e.shiftKey, this.state.editor); |
| onGutterClick = (cm, line, gutter, ev) => { | ||
| const { | ||
| selectedSource, | ||
| toggleBreakpoint, |
There was a problem hiding this comment.
why are we removing these? they seem to still be used the same way as before
There was a problem hiding this comment.
Ah, forgot to roll that back. Will take care of it. Sorry for the confusion.
| }; | ||
|
|
||
| openMenu(event) { | ||
| const { setContextMenu } = this.props; |
There was a problem hiding this comment.
we probably don't need this change
| continueToHere: Function, | ||
| setContextMenu: Function | ||
|
|
||
| // Actions |
There was a problem hiding this comment.
nice idea with the comment!
| getHitCountForSource, | ||
| getCoverageEnabled, | ||
| getConditionalPanelLine, | ||
| getFileSearchModifiers, |
|
Hey @amelzer is this good to go or are there other changes? |
|
@jasonLaster Pushed another reroll, good to go now |
98bcdc3 to
2ef82fb
Compare
|
just did a quick rebase. should be good |
|
@amelzer looks like there are some failing tests.. that need to be sorted out :/ |
…ger.html into new-redux-action-in-editor
|
@jasonLaster Tadaa |
|
@amelzer i see another conflict... sorry :/ |
|
That's it, I'm calling it quits in this PR. |
Associated Issue: #4435
Here's the Pull Request Doc
https://devtools-html.github.io/debugger.html/CONTRIBUTING.html#pull-requests
Summary of Changes