-
Notifications
You must be signed in to change notification settings - Fork 110
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
Feature/table #18
base: main
Are you sure you want to change the base?
Feature/table #18
Conversation
…feature/table # Conflicts: # src/index.js
Looks good - we'll definitely need to inline the reduce since that's costly. Also the spacing seems off - there's an |
@developit Sorry about the spacing didn't notice it. I removed the spacing issue. I'll look into optimization tomorrow. |
Awesome, thanks! |
@developit The reduce function doesnt seem that costly in this situation: This evening I'll replace the v.trim() with parse(v), then it should allow inline styling |
@developit might have found a way to minimize it, by combining the header and content. I'll try it out later. |
@developit Shaved off 10 bytes. I think I reached my personal limit. |
LGTM! |
Looking good! I'll take a look at this locally. |
Side note on reduce: I'm seeing vast speed improvements in .reduce since node v0.12/v4 days. Extra mem allocs are mostly cleaned up in the faster GC pass. Both for string & array return types! The only reason I avoid now is for closer WASM compat. Either way, great work @ShynRou - super excited to see this get added! |
Still not merged... I have written my own tiny markdown parser in the meantime. So if you need Table or recursive List support: https://github.com/shynrou/micro-down |
i = l.length, | ||
table = '', | ||
r = 'td>'; | ||
while ( i-- ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might want to remove the spaces inside the parentheses for consistency with the rest of the file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and to save some bytes ofc ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed them for code-styles sake
@tbjgolden since it's minified spaces don't add to final size
@developit what help do you need to get this merged? I may help and I'm pretty interested on having this merged. :) |
+1 !! |
Added table functionalty with optional header cells.
I'm not sure if it's the shortest solution but it works.