Skip to content

refactor: Use template literals instead of string concatenation when building popup HTML - #1464

Open
tora-pan wants to merge 3 commits into
melink14:mainfrom
tora-pan:tora-pan/refactor-makeHtml
Open

refactor: Use template literals instead of string concatenation when building popup HTML#1464
tora-pan wants to merge 3 commits into
melink14:mainfrom
tora-pan:tora-pan/refactor-makeHtml

Conversation

@tora-pan

@tora-pan tora-pan commented Mar 19, 2023

Copy link
Copy Markdown
Contributor

The inline variable replacement of template literals allows us to remain the desired HTML shape of the string.

Renamed variables for better readability.

@codecov

codecov Bot commented Mar 19, 2023

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 80.82192% with 28 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.86%. Comparing base (905358e) to head (52467cb).
⚠️ Report is 1423 commits behind head on main.

Files with missing lines Patch % Lines
extension/data.ts 80.82% 28 Missing ⚠️

❌ Your patch check has failed because the patch coverage (80.82%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1464      +/-   ##
==========================================
+ Coverage   79.62%   79.86%   +0.23%     
==========================================
  Files           7        7              
  Lines        3004     3005       +1     
  Branches      189      189              
==========================================
+ Hits         2392     2400       +8     
+ Misses        607      600       -7     
  Partials        5        5              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@melink14 melink14 changed the title refactor: Use template strings to read more like HTML refactor: Use template literals instead of string concatenation when building popup HTML Mar 19, 2023

@melink14 melink14 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems fine overall but will need to take a block of time to study it since it produced a non-trivial diff.

Since several of the branches don't have tests (not your fault) we might want to add those first so we can be confident in the refactor. (I can do it as well if you don't have time or desire)

Finally, I agree the variable renaming is good but I would probably have done that in it's own PR first since it leads to cleaner diffs and easier to review PR. That said, it's only a minor inconvenience in this case so it's not worth struggling with git to split it! It's something to consider in the future though!

I also updated the title and body a little bit to have the title focus on exactly what was done and the body give extra context for why (and thanks for separately mentioning renames! that's very important when combining changes!). It was very close already though so only needed minor tweaks ('need' is a strong word here)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants