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

Artist alias pages support #30

Closed
wants to merge 1 commit into from
Closed

Artist alias pages support #30

wants to merge 1 commit into from

Conversation

jesus2099
Copy link
Contributor

I want to add:

// @include      /^https?://((beta|test)\.)?musicbrainz\.org/artist/[0-9a-f-]{36}/(add-alias|alias\/\d+\/edit)(\?.+?)?(#.+?)?$/

Where slashes don't work if I don't protect them, at least inside this group.

I am not sure how this work, so let's see.
This PR works for artists, if it generates the include line I mention above.

Then I will add other entities than artist (but a quick try with release-group was not enough, I must change the text input field selector, apparently, as well, for other entities).

@kellnerd
Copy link
Owner

Thanks, supporting aliases is a good idea. I think I've even missed this feature once or twice but was to lazy to have a look at it.

The include rule is working as expected, even if I don't escape the path separator slashes with a backward slash (tested with FF+VM).
Unfortunately the script triggers React hydration errors for me on these pages and the buttons do not appear. If these did not happen it should work as far as I can see.
I think we need the new event from metabrainz/musicbrainz-server#2566 to solve this.

@kellnerd kellnerd added enhancement New feature or request punctuation Guess Unicode punctuation script labels Aug 19, 2022
@kellnerd
Copy link
Owner

Sorry for the long delay, but now that I've merged all of my MBS beta changes from #32 into the main branch (including c43a905), I had finally time to look into this properly.

In the punctuation-alias branch, I've implemented alias support for all entity types. Basically I only had to use the onReactHydrated() helper function in addition to your new include rule to make it work for artist aliases, but I decided to implement it as a separate case to be able to support all entity types and maybe even to detect the locale (which unfortunately behaves different from the release editor's language field) in a future release.

I would like to test this new version for a while before I merge it into the main branch, just to make sure that the new React helper doesn't cause any issue on other pages.

@jesus2099
Copy link
Contributor Author

jesus2099 commented Jan 26, 2023

Cancelling is irrevocable so I prefer asking.
Do you still need this merge request or should I cancel it? 😁

@kellnerd
Copy link
Owner

I guess you can cancel it. The code change is already part of my patch and our discussion will still be kept for future reference after closing the PR.

P.S. I've also added support for language specific rules based on the locale of an alias and fixed a mistake which I made yesterday and an even an old bug that I've found. So you have to install the version from the development branch again if you want to test it.

@jesus2099
Copy link
Contributor Author

No problem.

@jesus2099 jesus2099 closed this Jan 27, 2023
@kellnerd
Copy link
Owner

kellnerd commented Feb 3, 2023

I would like to test this new version for a while before I merge it into the main branch, just to make sure that the new React helper doesn't cause any issue on other pages.

The feature has been merged into main and should now be available as an automatic update.

@jesus2099 jesus2099 deleted the alias-pages-support branch February 3, 2023 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request punctuation Guess Unicode punctuation script
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants