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

Add "Save as HTML" to Command Palette. Resolves #432 #434

Merged
merged 8 commits into from
Oct 2, 2016
Merged

Add "Save as HTML" to Command Palette. Resolves #432 #434

merged 8 commits into from
Oct 2, 2016

Conversation

billglover
Copy link
Contributor

No description provided.

@billglover
Copy link
Contributor Author

This is my first attempt at adding a feature. This PR should add the "Save as HTML" command to the command palette along with two tests to check that it works in the following scenarios:

  • editor has the focus
  • preview pane has the focus

@billglover
Copy link
Contributor Author

AppVeyor job failures seem unrelated to this pull request: https://ci.appveyor.com/project/Atom/markdown-preview/history

Is anyone able to let me know if this isn't the case?


runs ->
expect(fs.isFileSync(outputPath)).toBe true

Copy link
Contributor

Choose a reason for hiding this comment

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

Extra newline

waitsForPromise -> atom.workspace.open("subdir/simple.markdown")
runs -> atom.commands.dispatch workspaceElement, 'markdown-preview:toggle'
expectPreviewInSplitPane()

Copy link
Contributor

Choose a reason for hiding this comment

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

Extra newline

return unless editor.getGrammar().scopeName in grammars

uri = @uriForEditor(editor)
previewPane = atom.workspace.paneForURI(uri)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rename this markdownPreviewPane to be more explicit (especially since there used to be a Tabs setting called "Preview Panes" or similar).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used previewPane to retain consistency with lines 75-77:

    previewPane = atom.workspace.paneForURI(uri)
    if previewPane?
      previewPane.destroyItem(previewPane.itemForURI(uri))

Happy to amend the lines relevant to #432 to use markdownPreviewPane but I assume lines 75-77 should be addressed as part of a new issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, if there's other places using this then let's keep it consistent.

Following code review by @50Wliu I have renamed variables to be more
explicit and reduce the possibility of confusion with a previous Tabs
setting called "Preview Panes" or similar.
activePane = atom.workspace.getActivePaneItem()

if isMarkdownPreviewView(activePane)
activePane.saveAs(). then ->
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the extra space before then there for a reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No reason at all, a miss on my part. Thanks for highlighting it.


runs ->
spyOn(atom, 'showSaveDialogSync').andReturn(outputPath)
console.log(outputPath)
Copy link
Contributor

Choose a reason for hiding this comment

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

Last thing: 🔥 this line.

@winstliu
Copy link
Contributor

winstliu commented Oct 2, 2016

@billglover You got the other console.log but not the one that my comment was on :P.

@billglover
Copy link
Contributor Author

billglover commented Oct 2, 2016

Noted! console.log() in tests is bad. Warnings during normal operation seem fine, i.e. console.warn('Copying Markdown as HTML failed', error) in copyHtml here.

@winstliu
Copy link
Contributor

winstliu commented Oct 2, 2016

Ideally there should be no uses of console at all, but that code was probably written before the Notifications package was created.

Thanks for this!

@winstliu winstliu merged commit 05a5adc into atom:master Oct 2, 2016
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