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

Move badge templates from doT โ†’ Template literals #2428

Closed
3 tasks
RedSparr0w opened this issue Nov 29, 2018 · 9 comments
Closed
3 tasks

Move badge templates from doT โ†’ Template literals #2428

RedSparr0w opened this issue Nov 29, 2018 · 9 comments
Labels
blocker PRs and epics which block other work npm-package Badge generation and badge templates

Comments

@RedSparr0w
Copy link
Member

๐Ÿ“‹ Description
#2394 (comment)
Rewrite the badge templates to use template literals.

๐ŸŽฏ Objective

  • Improve readability
  • Improve performance
  • Merge some parts of the templates to reduce duplication

โ˜‘๏ธ TODO

  • Write basic template
  • Test performance vs doT
  • Optimize
@paulmelnikow paulmelnikow added the core Server, BaseService, GitHub auth, Shared helpers label Nov 29, 2018
@paulmelnikow
Copy link
Member

Thanks for opening this.

I'm marking this "good first issue." This is not the most straightforward task, though for someone who is comfortable with JavaScript and unit testing, they probably would be able to accomplish this without a lot of knowledge of Shields. It'd be a good introduction to the project, and also very high value. And, of course it's a great thing for an existing contributor or maintainer to take on, too. ๐Ÿ˜„

@paulmelnikow paulmelnikow added good first issue New contributors, join in! npm-package Badge generation and badge templates and removed core Server, BaseService, GitHub auth, Shared helpers labels Nov 29, 2018
@RedSparr0w
Copy link
Member Author

I've made an example here using the converted templates:
Shields.io Badge Generator

Convert the doT template files to template strings:

template.replace('<svg', 'return `<svg')
  .replace(/\{\{=(.+?)\}\}/g, "${$1 ? `$2` : ''}")
  .replace(/\{\{\?(.+?)\}\}(.+?)\{\{\?\}\}/g, "${$1}")
  .replace(/\{\{\?(.+?)\}\}\r?\n(\s+)/g, "${$1 ?\n$2`")
  .replace(/\r?\n(\s+)\{\{\?\}\}/g, "`\n$1: ''}")
  .replace('</svg>', '</svg>`;');

Not sure how to go about it in shields though, will look into it tomorrow (hopefully)

@paulmelnikow
Copy link
Member

What do you think about this approach? Placeholder logic, though it conveys the ideaโ€ฆ

const hasLeftText = text[0].length > 0
const [leftWidth, rightWidth] = ...
const leftPadding = ...
const totalWidth = leftWidth + leftPadding + leftWidth

return `
<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="${totalWidth}" height="20">
...
`

@RedSparr0w
Copy link
Member Author

RedSparr0w commented Dec 4, 2018

I've updated the CodePen using a similar approach, Not sure how to tidy it up more though:
https://codepen.io/redsparr0w/full/PxgPdG/

@paulmelnikow
Copy link
Member

Looks like progress!

Do you feel like playing with it further? I suppose the next things I'd suggest are destructuring it, and hoisting out more of the compound expressions like 10+it.logoWidth+it.logoPadding and leftWidth+rightWidth/2-(it.text[0].length ? 1 : 0 ))*10.

@paulmelnikow paulmelnikow added the blocker PRs and epics which block other work label Jan 8, 2019
@mbarkhau
Copy link
Contributor

mbarkhau commented Feb 24, 2019

I've done some initial investigation into the possible performance improvements. From what I can tell there is not much to be gained here.

https://codepen.io/mbarkhau/pen/xMvvRd?editors=0010
https://jsperf.com/dot-vs-template-literals/1

Of course I may be doing an unfair comparison, but even if that is the case, I doubt that performance would be the main justification for this change vs the other objectives.

@paulmelnikow
Copy link
Member

Thanks for looking into that! I think improving readability, DRY, and eliminating the mental overhead of another library are good reasons to do it.

@paulmelnikow
Copy link
Member

Hey @RedSparr0w, mind if I pick this up?

paulmelnikow added a commit that referenced this issue Apr 30, 2019
While working on #2428 I found myself wanting to reload the server frequently. This is working great and reducing my iteration time significantly. I should have tackled this way sooner! :P

Iโ€™ve left verbose on which seems useful, at least in the short term while weโ€™re tuning the configuration.

Close #2426
paulmelnikow added a commit that referenced this issue Apr 30, 2019
While working on #2428 I found myself wanting to reload the server frequently. This is working great and reducing my iteration time significantly. I should have tackled this way sooner! :P

Iโ€™ve left verbose on which seems useful, at least in the short term while weโ€™re tuning the configuration.

Close #2426
@paulmelnikow
Copy link
Member

I'm making good progress on this. I should have a PR up sometime in the next couple of days.

It doesn't have exact parity right now, or run through the optimizer, so there is a bit of testing that needs to be done to make sure nothing is broken.

paulmelnikow added a commit that referenced this issue May 1, 2019
While working on #2428 I found myself wanting to reload the server frequently. This is working great and reducing my iteration time significantly. I should have tackled this way sooner! ๐Ÿ™Š 

Iโ€™ve left `verbose` on which seems useful, at least in the short term while weโ€™re tuning the configuration.

Close #2426
@paulmelnikow paulmelnikow removed the good first issue New contributors, join in! label Oct 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocker PRs and epics which block other work npm-package Badge generation and badge templates
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants