-
Notifications
You must be signed in to change notification settings - Fork 395
UI/TagInput: 45566, raw-encode tags #9941
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
Conversation
7337d15 to
9dec0f1
Compare
2913a84 to
56f099a
Compare
|
Just to put it on the radar: This will collide rather painfully with #9516 . Thank you very much and best, |
|
The relevant changes are mainly outside the js, it will collide, yes, but not that hard :) |
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.
Hi @nhaagen,
Thanks a lot for your contribution to the UI components!
As @kergomard pointed out, this PR conflicts with his PR to introduce auto-completion to the Field\Tag component. He agreed to incorporate your PR into his, since the changes on his end are less trivial and use a different structure. Could you implement the requested changes and pass the PR on to @kergomard once you finished? He will then incorporate and close this PR.
About the concrete changes, please answer the following questions:
- Relevant changes: You have migrated the JavaScript to an ES6 module, thx! To make sure we don't miss any relevant changes, could you point out which changes address the Mantis issue in question? As far as we can see, it should be three
rawurlencode()and oneencodeURIComponent()function calls.
Please implement the following changes:
- Unit tests: please implement a server-side unit test which covers the bug you are fixing. @kergomard will implement the client-side unit tests inside his new JS structure.
Kind regards,
@thibsy
|
Just to be clear: As far as I understand this, I would just throw away al changes in the Javascript, and only keep the three small changes adding If there is something else to do that I overlooked when I quickly looked at this, I would add it in the implementation I provided for the js, as there the change is far bigger and I would actually simply suggest that I close this PR once I've done so. Best, |
|
Hi @kergomard, There was indeed a misunderstanding. We can continue as you suggested - I updated my review accordingly. Kind regards, |
56f099a to
7c9c39f
Compare
For the JS, This encodeURI becomes encodeURIComponent here in transformTag. |
thibsy
left a comment
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.
Thx @nhaagen for providing the PHPUnit test. I approve these changes and let @kergomard incorporate them into his PR.
https://mantis.ilias.de/view.php?id=45566