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

Implement markdown parser to convert markdown to html #949

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@kunall17
Contributor

kunall17 commented Jul 30, 2017

  • Emoji parser

Taken files raw files fenced_code.js, markdown.js, marked.js, util.js, hash_util.js

To be used in Full Screen editor and local echo of messages when sending message

kunall17 added some commits Jul 29, 2017

Add raw files from zulip/zulip fenced_code.js, markdown.js, marked.js…
…, util.js, hash_util.js

All files are from commit 016167198143a1f518f8a141d1aadeeacb5f3a87
@smarx

This comment has been minimized.

smarx commented Jul 30, 2017

Automated message from Dropbox CLA bot

@kunall17, it looks like you've already signed the Dropbox CLA. Thanks!

@borisyankov

This comment has been minimized.

Contributor

borisyankov commented Jul 30, 2017

We should publish the markdown as a library.
This is too messy, code that completely does not conform to our coding standards, uses outdated forEach lodash etc.

@kunall17

This comment has been minimized.

Contributor

kunall17 commented Jul 31, 2017

Ok, @timabbott can you help me creating a repo zulip/zulip-markdown-parser perhaps?

@timabbott

This comment has been minimized.

Member

timabbott commented Jul 31, 2017

I'm happy to create a repo and give you admin access to it, but doing any sort of integration to make the server project use it is probably out of scope for what I have time to help with in the next few weeks. I think there's 3 valid strategies for the near term:

  • Basically include things as Kunal has, using lint-ignore files to just exclude the included code from our coding standards in the RN project. That would let us get the integration correct and the RN app feature done without blocking on a big refactoring. And then we can do the work to extract that as its own package much later. Whether this is a good approach depends somewhat on how easy it is to do the style excludes.
  • Just create a branch against zulip.git with the packaging (+ maybe some stubbing) changes and build+publish that to NPM (without it having its own repository or other infrastructure). We'd rebase and re-publish the package as needed when we make changes we want to pull into mobile. I think this is what Boris had in mind?
  • Work in a new repository. This feels cleaner, but also involves by far the most setup and maintenance work of the options, so I'd prefer we go one of the other routes.
@borisyankov

This comment has been minimized.

Contributor

borisyankov commented Jul 31, 2017

I vote for option 2 in the short run.
@kunall17 if you don't know how to publish an npm package, I am sure you can figure it out, but also ask me if you have any issues.

@kunall17

This comment has been minimized.

Contributor

kunall17 commented Jul 31, 2017

Ok, I'll create this is a package, just the emoji work has been left!

@kunall17

This comment has been minimized.

Contributor

kunall17 commented Jul 31, 2017

Published this as a package https://www.npmjs.com/package/zulip-markdown-parser
Works well :)

Pushed to a new repo as well for now https://github.com/kunall17/zulip-markdown-parser
Has to be pushed to the zulip's branch, or maybe you could create a orphaned branch and I can send a PR that will ease the review as well!

@kunall17 kunall17 changed the title from [WIP] Implement markdown parser to convert markdown to html to Implement markdown parser to convert markdown to html Aug 4, 2017

@zulip zulip deleted a comment from zulipbot Aug 4, 2017

@zulipbot

This comment has been minimized.

Member

zulipbot commented Aug 4, 2017

Heads up @kunall17, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

@borisyankov

This comment has been minimized.

Contributor

borisyankov commented Aug 5, 2017

@kunall17 do we need this PR since you published as a package?

@kunall17

This comment has been minimized.

Contributor

kunall17 commented Aug 7, 2017

Nope!

@kunall17 kunall17 closed this Aug 7, 2017

@zulipbot zulipbot removed the needs review label Aug 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment