Delete selected mocks and groups with keyboard #222
Conversation
There was a problem hiding this comment.
Hi!
Thanks so much for this PR, i've written some notes on things that should be changed.
Also there is another challenge with this issue is that we need to take care of.
The current issue is if I press delete or backspace anywhere on the screen except when in focus on the editor, the selected items will be deleted.
I think for now we might want to check if the user mouse is hovering over the MockSidebar component or any of it's children and only then process the delete operation.
@ilyagelman I'd love your thoughts about this on how we can do this without causing the user to accidentally delete mocks.
Regarding tests:
Currently we do not have UI tests but just API and integration tests that check the core functionality of this library (being able to mock requests).
We'd love to bring in jest and start testing UI components, I'll create an issue for it so we can handle it in it's own context.
| } | ||
|
|
||
| componentWillUnmount() { | ||
| API.off(EVENTS.UPDATE_MOCK, this.reRender); | ||
| API.off(EVENTS.UPDATE_GROUP, this.reRender); | ||
|
|
||
| document.removeEndEventListener("keydown", this.onKeyDown); |
There was a problem hiding this comment.
should be removeEventListener instead of removeEndEventListener
| } | ||
|
|
||
| onKeyDown = (event) => { | ||
| if (event.keyCode === KEYCODES.BACKSPACE || event.keyCode === KEYCODES.DELETE) { | ||
| const selectedGroupsIds = this.props.selectedGroups.map(group => group.id); |
There was a problem hiding this comment.
There is no need to do this logic yourself, we have State classes to handle these mutations so we can have them all in one place.
You can import MocksState which is the state for anything related directly to mocks (and groups) and use the deleteSelected method.
There was a problem hiding this comment.
I guess we have to document all the states we have
|
@morsdyce Good question about focus... Obviously we would need undo for this. What about adding a |
|
Good idea, I like it much better than assuming that if the mouse is in the sidebar area then you probably meant to delete. I think this is the proper solution for now, undo will be important for this feature so we might want to release them together. |
dmitrika
force-pushed
the
Delete-selected-mocks-and-groups-with-keyboard
branch
from
b449cfa
to
7a89654
Compare
dmitrika
force-pushed
the
Delete-selected-mocks-and-groups-with-keyboard
branch
from
7a89654
to
4875afd
Compare
|
Done with typo and MocksState. I got idea with focus, all right. And how particularly you see undo logic here? Is this a button or cmd + z hotkey? Or gmail like popover? |
|
We have a button which is hidden right now because the functionality isn't ready which might also be bound to a hotkey |
|
@dmitrika I'd tackle the undo as a different issue, doesn't have to be done now |
|
I'm still want to finish the task, but waiting when I would be able to get DOM-node from styled-components and add 'click' event-listener to it. Now it's kinda broken styled-components/styled-components#629 |
|
I added Unfortunately, I couldn't get DOM-element from styled-components, despite bugfix, so I added cb to DnD component to get node from it. |
|
Thanks! I just went on vacation so will take me a while to review it.
…On Apr 4, 2017 15:05, "Dmitry Rybin" ***@***.***> wrote:
I added sidebarInFocus flag and toggle function to the MocksState.
Unfortunately, I couldn't get DOM-element from styled-components, despite
bugfix, so I added cb to DnD component to get node from it.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#222 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABlqItkGP0z4x5hTZWjM5ULnkqJbIyliks5rsqKSgaJpZM4Mhwwu>
.
|
|
@morsdyce Just want to ping you in case if you missed pr :) |
|
@dmitrika Don't worry, I didn't forget about you, I just came back from a month long vaction and have to deal with some work catch up stuff as well as preparing a few things to present mimic in oscon. I will check this as soon as I can which should be in the next few days. Thanks for your help! |
|
@dmitrika I just checked it and it works beautifully! |
|
Great, thanks! |
* next: Feat (editor): Add codemirror plugins for closing and matching brackets (#249) Fix (mocks): Allow duplicating a mock in the same group (#248) chore(UI): Upgrade React PropTypes to the new prop-types package chore(Build): Upgrade dependencies refactor(API): exported Group, Mock and Request models in the API levle Chore (npm scripts): Remove postinstall script as I think it's a bit too much installing a dependency and showing this on every install Added call to donate after npm install Added backers and sponsors on the README Support loading mimic inside <head> tag Release v2.0.2 Add a small paragraph to clarify what this tool does; Feat (Mocks Sidebar) Allow to delete selected mocks and groups with keyboard shortcuts (#222)
Hello!
This pr is connected with #216.
It's my first contribution with code-changes, so feel free to point where I should correct it.
And I couldn't get how to cover it with test, any thoughts?