UX: Allow saving with cmd+s/ctrl+s with Ace editor (PR #12674)

When editing the files for a theme in the admin dashboard, typing “cmd+s” (a common key-binding to save in most text editors) used to engage the browser’s default “save page” dialogue.

This commit adds a key-binding to the ace editor that saves the file.

Now, the “cmd+s” (and “ctrl+s” for windows) key-binding does the same action as the save button.

Note: I didn’t write a test for this because I’m not sure where the JS tests are located. I can write a test if needed as long as someone points me in the right direction.

GitHub

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

Awesome! Have you tested how this will affect ace-editor instances other than admin-theme-editor?

Off the top of my head, I think the data-explorer plugin uses ace-editor. Possibly some other places in core as well.

Maybe this should be something like

if(this.attrs.save){
  this.attrs.save()
}

So that it doesn’t throw an exception if the save attribute isn’t passed?

This was my first thought and it does prevent an error from showing up if the parent component/controller doesn’t have a ‘save’ action.

However, I’m thinking of putting the entire command declaration within that conditional like this –

if (this.attrs.save) {
  editor.commands.addCommand({
    name: "save",
    exec: () => {
      this.attrs.save();  
    },
    bindKey: { mac: "cmd-s", win: "ctrl-s" },
  });
}

That way, if the parent doesn’t define a save() action, the browser default save-page dialogue still pops up.

Good thinking @davidtaylorhq, the ace editor is also used in the email style editor and the badge_sql editor. I’m working on a fix for this and trying to identify the cause of the backend error that failed the CI check.

Thanks!!

trying to identify the cause of the backend error that failed the CI check

I think we might have just been unlucky there. I restarted the tests and it seems fine now :grinning_face_with_smiling_eyes:

I think we might have just been unlucky there. I restarted the tests and it seems fine now :grinning_face_with_smiling_eyes:

Ah okay, great!

I just tested that new code with and without a save attribute. When it’s included, the key-binding calls whatever save function was passed as an attribute. And when it’s not included, it doesn’t register the command and the browser default is run.

I just pushed the latest edits which address this (and also add the keybinding to the email-style-editor).

Do you want me to write a test for this? Or do you think it’s straightforward enough?

The title of this pull request changed from “UX: Allow saving with cmd+s/ctrl+s on admin/customize/theme/editor” to "UX: Allow saving with cmd+s/ctrl+s with Ace editor

Looks great, sorry for the delay here @graydenshand. Merging now :slight_smile: