Skip to content
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

WIP: Support for save and save as #24

Merged
merged 18 commits into from
Jan 5, 2018
Merged

WIP: Support for save and save as #24

merged 18 commits into from
Jan 5, 2018

Conversation

kethinov
Copy link
Member

@kethinov kethinov commented Dec 29, 2017

Closes #21

Closes #25

Closes #26

Probably closes #14

Makes progress on #4

@kethinov
Copy link
Member Author

Depends on glyphr-studio/Glyphr-Studio-1#263

@kethinov
Copy link
Member Author

@mattlag and @Autre31415 this is a super rough implementation so lemme know what sucks and I'll fix it.

One thing I don't like about it already is it only works via keyboard commands (ctrl/cmd+s for save and ctrl/cmd+shift+s for save as). The visual save button is not handled.

Open to ideas for improvements on how it should work.

@mattlag
Copy link
Member

mattlag commented Dec 29, 2017

I agree, this needs a bit more thought.

  1. Handle it via the (Electron-only) File > Save As option. File menu doesn't exist in the browser version, so could be surfaced / handled separately here.

  2. Treat the save button in the left bar as "Save As" if electron, but if browser then treat as current save experience.

@kethinov
Copy link
Member Author

Okay yeah that all makes sense. I'll push an update with those changes.

Can you publish a new version of the main repo that has my changes to it so we can inherit it in the desktop app?

@kethinov
Copy link
Member Author

All right, pushed up some changes that reflect those additional requirements.

Pretty hacky in how it interacts with that save button, but it seems to work. If we want to clean up the interactions between how electron talks to that save button, I'll need to PR the main project again. Up to you which way you think we should go with it.

dialog.showSaveDialog({
properties: ['openFile'],
title: 'Choose where to save project...',
defaultPath: process.env.HOME + '/' + fname
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

} else {
event.returnValue = 'false'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without a replacement for this line, selecting "Cancel" on the "Would you like to save before closing?" dialog will always cause the app to close.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed the bug associated with this

dialog.showSaveDialog({
properties: ['openFile'],
title: 'Choose where to save project...',
defaultPath: process.env.HOME + '/' + fname
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@kethinov
Copy link
Member Author

I suppose one more thing I should do on this at least before we merge it is suppress the save/save-as menus on the welcome screen.

@kethinov
Copy link
Member Author

Okay, took care of suppressing the save/save-as menus on the welcome screen.

@kethinov
Copy link
Member Author

@mattlag and @Autre31415 I think I'm done tweaking it now. Give it a spin and let me know if you think this is good now.

@Autre31415 Autre31415 merged commit de64a3b into glyphr-studio:master Jan 5, 2018
@kethinov kethinov deleted the save-save-as branch January 5, 2018 19:29
@Autre31415
Copy link
Member

@mattlag I've issued a release that includes these changes here. If you think any other changes should be made let us know and we'll put up another PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants