Skip to content
This repository has been archived by the owner on Apr 1, 2020. It is now read-only.

Markdown changes #2282

Merged
merged 15 commits into from
Jun 19, 2018
Merged

Markdown changes #2282

merged 15 commits into from
Jun 19, 2018

Conversation

CrossR
Copy link
Member

@CrossR CrossR commented Jun 4, 2018

A few tiny Markdown preview changes, to make the preview experience a bit more consistent.

Fixes #1277, #2021.

  1. Close the preview when swapping out of a MD file.
  2. Don't auto open the preview if it was manually closed. (Should stop it opening back up when a user closes it).
  3. Fix highlighting issue (ie Preview was highlighted on Open, not Editor)
  4. Fix link opening by having it use the Oni browser.

Would you be able to take a look over @TalAmuyal ?

EDIT: Now depends on a pending PR on oni-api due to the API changes for the focus / active split.

@codecov
Copy link

codecov bot commented Jun 4, 2018

Codecov Report

Merging #2282 into master will increase coverage by <.01%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2282      +/-   ##
==========================================
+ Coverage   37.34%   37.34%   +<.01%     
==========================================
  Files         298      298              
  Lines       12351    12352       +1     
  Branches     1632     1632              
==========================================
+ Hits         4612     4613       +1     
  Misses       7490     7490              
  Partials      249      249
Impacted Files Coverage Δ
...src/Services/Configuration/DefaultConfiguration.ts 87.5% <0%> (ø) ⬆️
...rowser/src/Services/WindowManager/WindowManager.ts 67.59% <100%> (+0.3%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 16c2eb5...21b7585. Read the comment docs.

TalAmuyal
TalAmuyal previously approved these changes Jun 5, 2018
Copy link
Collaborator

@TalAmuyal TalAmuyal left a comment

Choose a reason for hiding this comment

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

Looking good 👌

Before the fix was only applied to close.
@CrossR CrossR changed the title Markdown changes [WIP] Markdown changes Jun 5, 2018
@CrossR
Copy link
Member Author

CrossR commented Jun 5, 2018

I'm going to swap this back to WIP and add a fix for the highlighting issue as well (#1277).

@CrossR
Copy link
Member Author

CrossR commented Jun 11, 2018

This PR is waiting on my other PR in oni-api being merged.

@bryphe
Copy link
Member

bryphe commented Jun 18, 2018

Looks good to me, @CrossR ! Thanks for the fixes here 😄

I just brought in the oni-api changes - bumping the version so we can get a new NPM package for it.

@bryphe
Copy link
Member

bryphe commented Jun 18, 2018

Alright, looks like the new oni-api was published - oni-api@0.0.46. Looks like you'll need to bump the version here. Otherwise, looks good to me! 👍

@CrossR CrossR changed the title [WIP] Markdown changes Markdown changes Jun 19, 2018
@CrossR
Copy link
Member Author

CrossR commented Jun 19, 2018

Just fixing the merge conflict, then this should be fine to review/merge.

I've got a second Markdown PR to add syntax highlighting and fix relative paths for images, but I split that into a second PR as it was waiting on my PR to DefinitelyTyped to update the Marked type defs. I'll stick that up soon.

@@ -242,6 +242,11 @@ export function createWindow(
Log.info("...closed event completed")
})

currentWindow.webContents.on("will-navigate", (event, url) => {
event.preventDefault()
currentWindow.webContents.send("open-oni-browser", url)
Copy link
Member

Choose a reason for hiding this comment

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

One minor thing - we may not actually need a new ipc message in this case. I think we could use the execute-command one we have, along with the browser.openUrl action, something like:
currentWindow.webContents.send("execute-command", "browser.openUrl", url)

We do something similar in the menu:

executeMenuAction(browserWindow, { evt: "execute-command", cmd: ["browser.openUrl", url] })

@bryphe
Copy link
Member

bryphe commented Jun 19, 2018

Just had one minor item but not a blocker - change looks great to me! 👍 🎉 Thanks @CrossR !

@CrossR
Copy link
Member Author

CrossR commented Jun 19, 2018

I'll bring this in now and address the PR comment in my other markdown changes so this can be tested a bit.

@CrossR CrossR merged commit 7c09bcf into onivim:master Jun 19, 2018
@bryphe
Copy link
Member

bryphe commented Jun 19, 2018

Sweet! Thanks @CrossR! 💯

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.

Markdown Preview Dim Persists
3 participants