Skip to content
This repository has been archived by the owner on Dec 27, 2022. It is now read-only.

Prompt to save #1198

Merged
merged 8 commits into from
Oct 8, 2018
Merged

Prompt to save #1198

merged 8 commits into from
Oct 8, 2018

Conversation

Septimus
Copy link
Contributor

@Septimus Septimus commented Oct 8, 2018

Implemented a prompt when using CTRLorCMD + S on an HTML page. It grabs the page title as a default for file saving, but still gives you the option to update the filename and save path.

Fixed an issue that emerged when implementing this. I don't believe when downloading an `.html` page that we'd want the browser toolbar to open. The one with Downloads, History, etc. If we do want that to open, we can easily update this pull request to allow that.
Copy link
Member

@pfrazee pfrazee left a comment

Choose a reason for hiding this comment

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

Hey, looks good. A few questions and a couple of things about code style.

If we need to setup a standard formatter for the codebase then let's do that and have one pass, but I don't want the style to change incrementally in PRs.

app/background-process/ui/window-menu.js Outdated Show resolved Hide resolved
app/background-process/ui/window-menu.js Outdated Show resolved Hide resolved
app/background-process/ui/downloads.js Outdated Show resolved Hide resolved
app/background-process/ui/window-menu.js Outdated Show resolved Hide resolved
app/background-process/ui/window-menu.js Outdated Show resolved Hide resolved
@Septimus
Copy link
Contributor Author

Septimus commented Oct 8, 2018

Ye sorry, my vs code formats in save. I need to turn that off. I'll revert the format changes ASAP. Do you have a format file in the code, I'll look when I get home in 15mins or so.

In regards to the .html suppression I did this as I didn't feel when you download a .html file it needs to open the browser-dropdown-menu. The flow looks weird. Especially since most users will download nearly all .html files in milliseconds.

@pfrazee
Copy link
Member

pfrazee commented Oct 8, 2018

@Septimus okay can you comment on the .html suppression explaining that?

Regarding the code formatting, I updated the electron3 branch to use the linter more aggressively. It now fixes by default and runs as a pre-commit hook/

Fixed an issue that emerged when implementing this. I don't believe when downloading an `.html` page that we'd want the browser toolbar to open. The one with Downloads, History, etc. If we do want that to open, we can easily update this pull request to allow that.
Removed the plethora of whitespace changes.
Added code comments about the suppression of `new-download` event.
@pfrazee
Copy link
Member

pfrazee commented Oct 8, 2018

👍 Looks great!

@pfrazee pfrazee merged commit 53b61a1 into beakerbrowser:electron3 Oct 8, 2018
@Septimus Septimus deleted the prompt-to-save branch October 8, 2018 20:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants