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

Rich Text and json2md args #24

Open
danielbastos11 opened this issue Jan 25, 2017 · 3 comments
Open

Rich Text and json2md args #24

danielbastos11 opened this issue Jan 25, 2017 · 3 comments

Comments

@danielbastos11
Copy link
Contributor

First all, forgive me for using a single issue for two completely different problems, but I ended up mixing the two things while working on them.

Rich text

I've been working on an update to add support for Rich Text editing from JSON. I don't know if that's useful for anyone else, but it's been really handy for me. It allows for constructs like that:

  json2md({
        text: ['This is what ', {italic: {bold: 'rich'}}, ' text should ',
               'look ', {strike: {bold: 'like'}}]
      }) 

which resolves to:

This is what rich text should look like

json2md args

I was wondering if it would be a good idea to remove the _type argument and add options instead. That would allow the use of options.compact, to define whether the resulting elements should be separated by a \n, fixing the problem (check line 87 and 100 on lib/converters.js) with trailing whitespaces that I mentioned on #21 .

I put together a list of reasons to get rid of _type:

  • It would simplify the API
  • Adding option as a third argument to the function wouldn't be so much of a burden
  • _type is really not necessary. The only use case for it is the converters.img function, but that wouldn't be a problem

On the other hand, changing the API could lead to broken dependants, but even that wouldn't be so much of a problem, since _type isn't part of the Public API and shouldn't be used by external libraries anyway.

I implemented these changes and made them available on my fork.
If only a subset of them is acceptable, that's totally alright!
Please let me know what you think.

@IonicaBizau
Copy link
Owner

I love that notation! 👍
Currently json2md doesn't support inline styling as JSON.

What is the _type argument? I'm not sure.

I guess you can create a pull request and review the code together. Anyways, I like the idea! 😁
From a quick look, this is backwards compatible, right?

@danielbastos11
Copy link
Contributor Author

_type is an argument used to enforce the use of a specific converter (see this)
The only place it's used is on this function, but I reimplemented it to work like this.

It's not 100% backwards compatible, because some libraries might be depending on _type, but they shouldn't be, since it's not part of the public API.

I didn't have the time to lint and review the code yet
I'll do it as soon as I can, and then I'll send the PR

@IonicaBizau
Copy link
Owner

@danielbastos11 Sorry for reply. Yes, makes sense. Indeed, much better! 👍

Since the _type is not documented anywhere, as you said, nobody should really use it.

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

No branches or pull requests

2 participants