alonrbar/easy-template-x

I wrote an extension that adds minimal HTML styling.

Closed this issue · 3 comments

cvaize commented

At this stage I needed to process the tags: <br>, <b></b>, <i></i>, <u></u>.
I am waiting for your feedback and suggestions.
https://github.com/cvaize/easy-template-x-extension-html

I plan to add lists, links, styling via style, headers from 1-6 levels later.

alonrbar commented

Hi, I'm on a vacation with no access to my laptop until the end of the month. Will take a look when I get back home. Thank you!

alonrbar commented

Hi again,

First of all, looks really nice!

Some points I've noticed:

  1. In the code example in the README file you can import createDefaultPlugins and then just append to it. Alternatively you can create a new TemplateHandlerOptions() and then append to it's plugins field.
  2. nit: In the same code example you created two handler variables...
  3. index.js is well organized, but personally I prefer splitting things to files to make it easier to read each of them. It makes a great difference in my eyes. I would go for htmlPlugin.js, htmlObject.js and probably a utils.js, or at least something like this.
  4. Instead of iterating the run nodes of the paragraph here you can use XmlNode.remove(runNode). like I'm doing here for instance.
  5. findChildByName already exists so you can use it instead of this one.
  6. Again, personal style but I prefer reverting the if logic to get reduced indentation so instead of
copyAttributes(fromNode, toNode) {
  if (fromNode.attributes) {
    // do stuff
  }
}

I'd go for

copyAttributes(fromNode, toNode) {
  if (!fromNode.attributes) {
    return;
  }
  // do stuff
}

This is a common idiom. Check this source for instance for a more elaborate example for why it's a good practice.
Same goes for the createRun function which can benefit from the practice as well.

If you want, I will be happy to review further and dig deeper into the code of HtmlObject.splitTextByTags, HtmlObject.build and HtmlPlugin.createRun after this first iteration is done.

alonrbar commented

Hi, closing this one for now. Feel free to reopen if you ever want to continue the discussion here.