-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Made "Show in OS" OS-specific #6982
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.
Instead of having an if for the register command, I think it would look better to do something like
var showInOS = Strings.CMD_SHOW_IN_OS;
if (brackets.platform === "win") {
showInOS = Strings.CMD_SHOW_IN_EXPLORER;
} else if (brackets.platform === "mac") {
showInOS = Strings.CMD_SHOW_IN_FINDER;
}
Before the we register all the commands and then just call CommandManager.register(showInOS, Commands.NAVIGATE_SHOW_IN_OS, handleShowInOS);.
The same thing could be applied to the exit command.
An alternative could be to set a string in Strings.js, but not sure about that.
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.
I thought about that, too, but then I saw how it is done above (exit command).
But yes, this sounds reasonable. Will change it tomorrow.
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.
Sure. We could use the same if to set the quit strings too.
|
BTW, it might be easier to not include the translations in the PR. If you do we need someone else to review them. |
|
@SAplayer @TomMalbran IMO, the strings look good. |
|
Just pushed a commit. |
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.
Could you place both if at lines 1552, so that we can then have all the commands together? You can also use the same if (brackets.platform === "win") for both showInOS and quitString.
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.
I think the register commands are curently sorted by command type (file, navigate, edit, ...), so this could cause ambiguity.
I'll wait for another opinion.
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.
I don't understand... I am saying about moving just the Strings declarations, not the CommandManager.register.
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.
Ah yes, must have understood you wrong.
Just pushed another commit.
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.
My point was to have it like this:
var quitString = Strings.CMD_QUIT,
showInOS = Strings.CMD_SHOW_IN_OS;
if (brackets.platform === "win") {
...(the rest of the if, else if code)
}
// Register global commands
CommandManager.register(Strings.CMD_FILE_OPEN, Commands.FILE_OPEN, handleFileOpen);
...(all the other commands)
CommandManager.register(showInOS, Commands.NAVIGATE_SHOW_IN_OS, handleShowInOS);
So that there is nothing in between the CommandManager.register and would make it easier to read.
|
This looks good. Thank you both. |
Made "Show in OS" OS-specific
|
Thank you @SAplayer -- this has annoyed me since the dawn of time! |
This is for #6970, it makes
CMD_SHOW_IN_OSOS-specific (Finderon Mac,Exploreron Win).@couzteau for the german strings