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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(split links): rewrite "MB: Paste multiple external links" to TypeScript, fix shortcomings #473

Merged
merged 13 commits into from Jun 12, 2022

Conversation

ROpdebee
Copy link
Owner

@ROpdebee ROpdebee commented Jun 11, 2022

Another day, another TypeScript rewrite? This one became neglected ever since I first wrote it 馃槄

Fixes #471, root cause was metabrainz/musicbrainz-server#2544. Fix was easy enough, but I decided to get rid of the event listeners and instead wrap MB's methods to intercept the links and do all of the processing there. I also got rid of the _reactInternals hack and just used normal event dispatching.

Fixes #43 (finally!) by moving the checkbox outside of the table so we don't have to delete and re-insert it all of the time.

Fixes #42: Now runs on all external links editors that I know of (edit pages, create/add pages, and inside of the nested iframe dialogs, also on the relationship editor).

Tested manually, no unit tests for this since I think an E2E test would be much better suited (whenever those arrive 馃檮)

Now back to neglecting this script for another year 馃檪

The script is fully broken at the moment, and there are multiple issues
that have been open for a while. Addressing these issues will possibly
require many changes, so we may as well rewrite to TS along the way.

Still some type errors, will be fixed later.
Also rewrote pretty much the whole thing.

Ref: metabrainz/musicbrainz-server#2544
@ROpdebee ROpdebee added bug Something isn't working enhancement New feature or request mb_multi_external_links labels Jun 11, 2022
@ROpdebee ROpdebee requested a review from kellnerd June 11, 2022 22:48
@ROpdebee ROpdebee marked this pull request as ready for review June 11, 2022 22:48
@codecov
Copy link

codecov bot commented Jun 11, 2022

Codecov Report

Merging #473 (a62da28) into main (e3d776f) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #473   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           46        46           
  Lines         1008      1020   +12     
  Branches       168       170    +2     
=========================================
+ Hits          1008      1020   +12     
Impacted Files Coverage 螖
src/lib/util/dom.ts 100.00% <100.00%> (酶)
src/mb_enhanced_cover_art_uploads/form.ts 100.00% <0.00%> (酶)
...c/mb_enhanced_cover_art_uploads/providers/vgmdb.ts 100.00% <0.00%> (酶)

Continue to review full report at Codecov.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update e3d776f...a62da28. Read the comment docs.

Copy link
Collaborator

@kellnerd kellnerd left a comment

Choose a reason for hiding this comment

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

I have to admit that I have not installed this script because I rarely have multiple links to paste.
So I only left a few remarks and did not test 馃槆

src/mb_multi_external_links/meta.ts Outdated Show resolved Hide resolved
src/mb_multi_external_links/meta.ts Outdated Show resolved Hide resolved
src/lib/util/dom.ts Outdated Show resolved Hide resolved
src/mb_multi_external_links/index.ts Outdated Show resolved Hide resolved
src/mb_multi_external_links/index.ts Outdated Show resolved Hide resolved
@ROpdebee ROpdebee requested a review from kellnerd June 12, 2022 17:06
@ROpdebee ROpdebee merged commit 93c3455 into main Jun 12, 2022
@ROpdebee ROpdebee deleted the ts-multi-external-links branch June 12, 2022 22:42
github-actions bot added a commit that referenced this pull request Jun 12, 2022
refactor(split links): rewrite "MB: Paste multiple external links" to TypeScript, fix shortcomings (#473)
github-actions bot added a commit that referenced this pull request Jun 12, 2022
refactor(split links): rewrite "MB: Paste multiple external links" to TypeScript, fix shortcomings (#473)
github-actions bot added a commit that referenced this pull request Jun 12, 2022
refactor(split links): rewrite "MB: Paste multiple external links" to TypeScript, fix shortcomings (#473)
@github-actions
Copy link

馃殌 Released 3 new userscript version(s):

  • mb_caa_dimensions 2022.6.12.2 in 0d669e3
  • mb_enhanced_cover_art_uploads 2022.6.12.5 in 978e947
  • mb_multi_external_links 2022.6.12 in 4bd2c01

@ROpdebee
Copy link
Owner Author

ROpdebee commented Jun 12, 2022

Why did it release new versions of ECAU and CAA Dims?? Edit Oh it's the input setter const that wasn't tree-shaken. Can probably easily be fixed, but not sure whether it's worth fixing and redeploying once again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working deploy:success enhancement New feature or request mb_multi_external_links
Projects
None yet
2 participants