-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: factor out showDeleteSpace listener #129
refactor: factor out showDeleteSpace listener #129
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR. Just a very small change regarding naming. Can you change this to showDeleteSpacePrompt
? The handler basically shows a prompt to the user, so it's better to keep to the naming of the channel. Thanks!
Hi @fboechats, will you be able to incorporate the changes? Thanks! |
Hi @fboechats, thanks for your changes! Could you please change the file name and squash all of the commits into one? With the message |
@@ -2,7 +2,7 @@ | |||
const { dialog } = require('electron'); | |||
const { RESPOND_DELETE_SPACE_PROMPT_CHANNEL } = require('../config/channels'); | |||
|
|||
const showDeleteSpace = mainWindow => () => { | |||
const showDeleteSpacePrompt = mainWindow => () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file should also be called showDeleteSpacePrompt
for consistency.
@@ -2,7 +2,7 @@ | |||
const { dialog } = require('electron'); | |||
const { RESPOND_EXPORT_SPACE_PROMPT_CHANNEL } = require('../config/channels'); | |||
|
|||
const showExportSpace = mainWindow => (event, spaceTitle) => { | |||
const showExportSpacePrompt = mainWindow => (event, spaceTitle) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file should also be called showExportSpacePrompt
for consistency.
@@ -2,10 +2,10 @@ | |||
const { dialog } = require('electron'); | |||
const { RESPOND_LOAD_SPACE_PROMPT_CHANNEL } = require('../config/channels'); | |||
|
|||
const showLoadSpace = mainWindow => (event, options) => { | |||
const showLoadSpacePrompt = mainWindow => (event, options) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file should also be called showLoadSpacePrompt
for consistency.
Hi @fboechats, just wanted to know if you were going to be able to address these changes / rebase, otherwise I can try and make them myself in order to go forward with these PRs. Thanks! |
Hi @juancarlosfarah , sorry for the delay again, just did the changes you asked. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! Thanks.
@fboechats, would it be possible for you to squash all the 12 commits into one? That way the commit history stays clean. Here is how to do it in case you haven't done it before (subtitle: The hard(er) and less flexible way): https://gist.github.com/patik/b8a9dc5cd356f9f6f980#the-harder-and-less-flexible-way The commit message can then be similar to the first one:
Many thanks! |
Factor out the showLoadSpacePrompt listener to its own file. refs #104
Hi @juancarlosfarah just squashed the commits, after we finished this I will keep the refactor |
Thanks @fboechats! I'm doing the merge manually in 50d2eb9 and 3b44fd7 to resolve conflicts and will close this and the other PRs. |
Factor out the showDeleteSpace listener to its own file.
refs #104