Ability to close windows from Spaces #23
Conversation
|
Hello @codedread, I implemented a close button in the button panel that closes the currently open space. I attempted to add unit tests, but I’m still building my testing experience and couldn’t complete them this time. :'D |
|
Thanks @Mariam-Saeed ! This is great, I can work on convincing an AI to write some unit tests, this patch looks pretty great! Please address the above comments and resolve branch conflicts (rebase to get my latest changes) and then send back and I can merge your PR! |
|
@codedread, can you check now? |
It looks like you may have updated your branch so there are no conflicts, but you haven't addressed my 3 review comments above. Thanks again. |
|
@codedread , oh sorry I couldn't see them. |
js/spaces.js
Outdated
| }); | ||
| } | ||
|
|
||
| async function performClose(windowId) { |
There was a problem hiding this comment.
Can you pls move this function to background.js (which is the thing that is managing windows and calling chrome.windows.*) and just have handleClose() calling chrome.runtime.sendMessage()?
I'd like to keep the architecture consistent.
js/spaces.js
Outdated
|
|
||
| // Export for testing | ||
| export { normaliseTabUrl }; | ||
| export { normaliseTabUrl, performClose, handleClose }; |
There was a problem hiding this comment.
nit: No need to expose the functions if there are no unit tests (for now).
js/spaces.js
Outdated
| const { windowId, name } = globalSelectedSpace; | ||
|
|
||
| // Only show confirm if the space is unnamed | ||
| const isUnnamed = !name || !name.trim(); |
There was a problem hiding this comment.
Instead of this checking, I think you can just see if the space has a sessionId instead.
|
Sorry my bad. I forgot to submit the review! |
|
@codedread Done |
2 participants
resolves #20